fix: idempotent cross-component message persistence + non-text-content policy (#89)

Closes the two remaining #89 acceptance criteria plus a related bug found
while investigating them:

- Neo4jChatHistoryProvider narrows MAF's default request-message filter to
  External-only, so it no longer re-persists another configured
  AIContextProvider's injected messages (e.g. Neo4jMemoryContextProvider's
  recalled-memory content) as new nodes every turn -- a real,
  previously-undiscovered duplication bug, not just a stated criterion.
- Message persistence is now idempotent by id (Neo4jMessageRepository writes
  MERGE instead of CREATE), and Neo4jMemoryContextProvider/
  Neo4jChatHistoryProvider/Neo4jChatMessageStore persist response messages
  under a deterministic id derived from the underlying ChatMessage.MessageId
  when the IChatClient populates one -- so independently-configured
  components observing the same response converge on one node instead of
  duplicating it. IMemoryIngestion.AddMessageWithIdAsync is added as a
  default interface method to stay backward-compatible; InstrumentedMemoryService
  gets a mandatory explicit override so the mechanism isn't silently defeated
  when observability is enabled.
- Documents and tests the non-text-content policy: a message carrying only
  function/tool calls, results, or reasoning content is excluded from
  persistence and extraction in the two MAF providers (not the lower-level
  Neo4jChatMessageStore path, which persists everything unconditionally).

Adversarially reviewed; full unit (2767) and live-Neo4j integration (285)
suites green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016FCv8MKpe8tTULe8M6JJGi

Author: joslat

CHANGELOG.md

@@ -50,10 +50,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   user-stated preference was invisible unless the assistant repeated it. Extraction now considers the
   complete turn. `Neo4jChatHistoryProvider` (which already persisted both request and response messages)
   gained full-provenance extraction from both at no new risk. `Neo4jMemoryContextProvider` deliberately
-  does not start persisting request messages as new `:Message` nodes (there is no idempotency mechanism to
-  avoid duplicating what a chat-history provider/store may already persist) — captured facts/preferences
-  are correct, but their provenance link to the literal source message is best-effort in that path, not
+  does not start persisting request messages as new `:Message` nodes — request-message persistence
+  ownership intentionally stays solely with `Neo4jChatHistoryProvider` — so captured facts/preferences are
+  correct, but their provenance link to the literal source message is best-effort in that path, not
   guaranteed; documented, not silently accepted.
+- **Enabling more than one MAF message-persisting component could duplicate `:Message` nodes (#89).**
+  `Neo4jChatHistoryProvider` now narrows MAF's default request-message filter to
+  `AgentRequestMessageSourceType.External` only, so it no longer re-persists another configured
+  `AIContextProvider`'s (e.g. `Neo4jMemoryContextProvider`'s) injected recalled-memory messages as new
+  nodes every turn — a real, previously-undiscovered bug found while investigating this issue, not merely
+  one of its stated acceptance criteria. On the response side, message persistence is now idempotent by
+  id: `Neo4jMessageRepository`'s writes are `MERGE`-by-id instead of `CREATE` (a no-op, first-write-wins,
+  for a repeated id — safe because `message_id` already has a uniqueness constraint, and no existing
+  caller ever supplied a repeated id), and `Neo4jMemoryContextProvider`/`Neo4jChatHistoryProvider`/
+  `Neo4jChatMessageStore` now persist a response message under a deterministic id derived from the
+  underlying `ChatMessage.MessageId` when the `IChatClient` populates one — so two of these components
+  observing the same response converge on one node instead of duplicating it. When the client doesn't
+  populate `MessageId`, today's fresh-id behavior remains (no regression, but the gap remains for that
+  client) — a disclosed limitation, not a silent one; see `docs/agent-framework.md`. Also explicitly
+  documents the non-text-content policy for `Neo4jMemoryContextProvider`/`Neo4jChatHistoryProvider`: a
+  message carrying only function/tool calls, function/tool results, or reasoning content is excluded from
+  both persistence and extraction in those two providers (previously true as a side effect of the
+  empty-text filter, now an explicit, tested contract for them specifically — the lower-level
+  `Neo4jChatMessageStore`/`Neo4jMicrosoftMemoryFacade` path does not have this guard and persists every
+  message it's given). #89 is now fully closed.
 
 ## [1.0.3] - 2026-07-14
 

docs/agent-framework.md

@@ -64,20 +64,36 @@ text isn't minted into spurious entities/facts/preferences every turn — so a p
 states is captured even if the assistant never repeats it back. `Neo4jChatHistoryProvider` persists both
 request and response messages as real `:Message` nodes (unchanged), so extraction there has full
 provenance. `Neo4jMemoryContextProvider` deliberately does **not** persist request messages as new nodes
-— only response messages are — because a host may already have `Neo4jChatHistoryProvider`,
-`Neo4jChatMessageStore`, or their own component persisting the same request messages on the same agent,
-and there is no idempotency mechanism (no caller-supplied message id, no upsert-by-content) that would let
-this provider safely avoid creating a duplicate `:Message` node for the same logical message. The
-practical effect: a fact/preference extracted from the user's own request is still created and recallable,
-but both its `EXTRACTED_FROM` provenance edge and its own `source_message_ids` property will reference a
-message id that was never persisted, unless another component also persisted that exact message.
-
-**This asymmetry is deliberate for requests, but note it does not (yet) extend to responses**: both
-`Neo4jMemoryContextProvider` and `Neo4jChatHistoryProvider` persist response messages unconditionally, with
-the same lack of an idempotency mechanism — so combining both providers (or either with
-`Neo4jChatMessageStore`/a custom component) on the same agent can still duplicate **response** `:Message`
-nodes today. Avoid wiring more than one message-persisting component onto the same agent until a real
-cross-component idempotency mechanism exists.
+— only response messages are — because a caller-constructed request `ChatMessage` essentially never
+carries a stable identity that another persisting component would also see, so request-message persistence
+ownership intentionally stays solely with `Neo4jChatHistoryProvider`. The practical effect: a
+fact/preference extracted from the user's own request is still created and recallable, but both its
+`EXTRACTED_FROM` provenance edge and its own `source_message_ids` property will reference a message id that
+was never persisted, unless another component also persisted that exact message.
+
+**Duplicate message persistence across components (#89)**: `Neo4jChatHistoryProvider` narrows MAF's
+default request-message filter to `AgentRequestMessageSourceType.External` only, so it never re-persists
+another configured `AIContextProvider`'s (e.g. `Neo4jMemoryContextProvider`'s) injected recalled-memory
+messages as new nodes every turn. On the **response** side, message persistence is idempotent by id:
+`Neo4jMemoryContextProvider`, `Neo4jChatHistoryProvider`, and `Neo4jChatMessageStore` all persist a response
+message under a deterministic id derived from the underlying `ChatMessage.MessageId` when the `IChatClient`
+populates one (true for many production clients, e.g. those backed by the OpenAI Responses API) — so if
+more than one of these components observes the same response message, they converge on the same
+`:Message` node instead of creating a duplicate. When the underlying client does **not** populate
+`MessageId`, each component still falls back to today's behavior (a fresh id per call), so combining more
+than one message-persisting component on the same agent can still duplicate that response message — this
+is a known, disclosed limitation of relying on provider-native identity rather than a cross-component
+idempotency protocol; there is no plan to add content-hash-based deduplication, since that would risk
+silently collapsing two genuinely distinct occurrences of identical text (e.g. the assistant saying
+"Understood." twice in different turns) into one node.
+
+**Non-text content policy**: a `ChatMessage` whose content is exclusively non-`TextContent` (e.g. a
+function/tool call, a function/tool result, or a reasoning trace) has an empty `.Text`, and `Neo4jMemoryContextProvider`/`Neo4jChatHistoryProvider` exclude it from both persistence and automatic
+extraction — not just "tool messages," but any message carrying no literal text for a human or the
+extraction pipeline to act on. This guard is specific to those two providers; the lower-level
+`Neo4jChatMessageStore`/`Neo4jMicrosoftMemoryFacade` path persists every message it's given regardless of
+text content, so a host driving that path directly is responsible for filtering non-text messages itself
+if it wants the same behavior.
 
 `Neo4jMicrosoftMemoryFacade` (the lower-level, manually-driven alternative to the context provider)
 does not yet wire configured `RecallOptions` into its own recall call — a known gap for a future pass, not

src/AgentMemory.Abstractions/Services/IMemoryIngestion.cs

@@ -19,6 +19,28 @@ Task<Message> AddMessageAsync(
         IReadOnlyDictionary<string, object>? metadata = null,
         CancellationToken cancellationToken = default);
 
+    /// <summary>
+    /// Adds a message to short-term memory with a caller-supplied, deterministic id. Calling this twice
+    /// with the same <paramref name="messageId"/> returns the pre-existing message unchanged (first-write-
+    /// wins) instead of creating a duplicate node -- lets independently-configured persisting components
+    /// (e.g. multiple MAF integration components observing the same underlying model response) converge on
+    /// one message node when they share a stable identity for it, instead of each minting a fresh one.
+    /// </summary>
+    /// <remarks>
+    /// Default implementation ignores <paramref name="messageId"/> and behaves exactly like
+    /// <see cref="AddMessageAsync(string,string,string,string,IReadOnlyDictionary{string,object}?,CancellationToken)"/>
+    /// (a fresh id every call) -- implementers of this interface are not required to override this member.
+    /// </remarks>
+    Task<Message> AddMessageWithIdAsync(
+        string sessionId,
+        string conversationId,
+        string role,
+        string content,
+        string messageId,
+        IReadOnlyDictionary<string, object>? metadata = null,
+        CancellationToken cancellationToken = default) =>
+        AddMessageAsync(sessionId, conversationId, role, content, metadata, cancellationToken);
+
     /// <summary>
     /// Batch adds messages to short-term memory.
     /// </summary>

src/AgentMemory.AgentFramework/Mapping/MafTypeMapper.cs

@@ -38,6 +38,17 @@ public static Message ToInternalMessage(
     public static ChatMessage ToChatMessage(Message message)
         => new(ToMafRole(message.Role), message.Content);
 
+    /// <summary>
+    /// Derives a deterministic persistence id from a response <see cref="ChatMessage"/>'s provider-native
+    /// <see cref="ChatMessage.MessageId"/>, when the underlying <c>IChatClient</c> populates one (common
+    /// for clients backed by e.g. the OpenAI Responses API). Returns null when absent -- caller-constructed
+    /// messages (typically request/user messages) essentially never have one, so this only helps dedupe
+    /// response messages that independently-configured persisting components (Neo4jMemoryContextProvider,
+    /// Neo4jChatHistoryProvider, Neo4jChatMessageStore) might otherwise each persist as a separate node.
+    /// </summary>
+    public static string? TryGetProviderMessageId(ChatMessage message) =>
+        string.IsNullOrWhiteSpace(message.MessageId) ? null : $"maf:{message.MessageId}";
+
     /// <summary>
     /// Converts a <see cref="MemoryContext"/> to a list of context <see cref="ChatMessage"/> instances.
     /// </summary>

src/AgentMemory.AgentFramework/Neo4jChatHistoryProvider.cs

@@ -39,7 +39,17 @@ public Neo4jChatHistoryProvider(
         ILogger<Neo4jChatHistoryProvider> logger,
         IMemoryStoreContext? storeContext = null,
         IWritableMemoryOwnerContext? ownerContext = null)
-        : base(null, null, null)
+        // storeInputRequestMessageFilter narrows MAF's own default (which excludes only ChatHistory-sourced
+        // messages) down to External-only. Without this, when a host also configures an AIContextProvider
+        // (e.g. Neo4jMemoryContextProvider) on the same agent, that provider's injected messages (recalled
+        // memory, wrapped as system-role content) show up in RequestMessages here too and get persisted as
+        // brand-new :Message nodes every single turn (#89). Trade-off: a host using a DIFFERENT,
+        // legitimate AIContextProvider whose injected content they actually want captured as persisted
+        // history would now have it silently excluded too -- treating provider-injected context as
+        // ephemeral (not history) is the correct default for this library, but it is a real behavior
+        // change from MAF's own out-of-the-box default for that uncommon case.
+        : base(null, static msgs => msgs.Where(
+            m => m.GetAgentRequestMessageSourceType() == AgentRequestMessageSourceType.External), null)
     {
         _memoryService = memoryService ?? throw new ArgumentNullException(nameof(memoryService));
         _clock = clock ?? throw new ArgumentNullException(nameof(clock));
@@ -140,18 +150,31 @@ internal async Task PerformStoreAsync(
                 storedRequests.Add(stored);
             }
 
-            // Persist response messages
+            // Persist response messages. When the underlying IChatClient stamps a provider-native
+            // MessageId, persist under a deterministic id (#89) so another persisting component observing
+            // the same response (e.g. Neo4jMemoryContextProvider, Neo4jChatMessageStore) converges on the
+            // same :Message node instead of creating a duplicate. Falls back to today's fresh-id behavior
+            // when absent.
             var storedResponses = new List<Abstractions.Domain.Message>();
             foreach (var msg in responseMessages)
             {
                 if (string.IsNullOrWhiteSpace(msg.Text)) continue;
-                var stored = await _memoryService
-                    .AddMessageAsync(
-                        sessionId, conversationId,
-                        MafTypeMapper.ToInternalRole(msg.Role),
-                        msg.Text,
-                        cancellationToken: cancellationToken)
-                    .ConfigureAwait(false);
+                var providerId = MafTypeMapper.TryGetProviderMessageId(msg);
+                var stored = providerId is not null
+                    ? await _memoryService
+                        .AddMessageWithIdAsync(
+                            sessionId, conversationId,
+                            MafTypeMapper.ToInternalRole(msg.Role),
+                            msg.Text, providerId,
+                            cancellationToken: cancellationToken)
+                        .ConfigureAwait(false)
+                    : await _memoryService
+                        .AddMessageAsync(
+                            sessionId, conversationId,
+                            MafTypeMapper.ToInternalRole(msg.Role),
+                            msg.Text,
+                            cancellationToken: cancellationToken)
+                        .ConfigureAwait(false);
                 storedResponses.Add(stored);
             }
 

src/AgentMemory.AgentFramework/Neo4jChatMessageStore.cs

@@ -40,9 +40,18 @@ public async Task<Message> AddMessageAsync(
         try
         {
             var message = MafTypeMapper.ToInternalMessage(chatMessage, sessionId, conversationId, _clock, _idGenerator);
-            return await _memoryService
-                .AddMessageAsync(message.SessionId, message.ConversationId, message.Role, message.Content, message.Metadata, cancellationToken)
-                .ConfigureAwait(false);
+            // When the underlying IChatClient stamps a provider-native MessageId on this ChatMessage,
+            // persist under a deterministic id (#89) so another persisting component observing the same
+            // message (Neo4jMemoryContextProvider, Neo4jChatHistoryProvider) converges on the same
+            // :Message node instead of creating a duplicate.
+            var providerId = MafTypeMapper.TryGetProviderMessageId(chatMessage);
+            return providerId is not null
+                ? await _memoryService
+                    .AddMessageWithIdAsync(message.SessionId, message.ConversationId, message.Role, message.Content, providerId, message.Metadata, cancellationToken)
+                    .ConfigureAwait(false)
+                : await _memoryService
+                    .AddMessageAsync(message.SessionId, message.ConversationId, message.Role, message.Content, message.Metadata, cancellationToken)
+                    .ConfigureAwait(false);
         }
         catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
         {

src/AgentMemory.AgentFramework/Neo4jMemoryContextProvider.cs

@@ -197,15 +197,14 @@ internal async Task PerformStoreAsync(
         try
         {
             // Response messages are persisted as new :Message nodes, as before. Request messages are
-            // deliberately NOT persisted here (#89): a host may already have a Neo4jChatHistoryProvider,
-            // Neo4jChatMessageStore, or their own component persisting the same request messages on this
-            // agent, and there is no idempotency mechanism (no MERGE-by-id, no caller-supplied id) that
-            // would let this provider safely avoid creating a second, duplicate :Message node for the
-            // same logical message. Building transient (never-persisted) Message objects for extraction
-            // only avoids that risk entirely while still capturing what the user said. Filtered to
-            // ChatRole.User -- the same filter recall already applies (BuildContextAsync above) -- so a
-            // system prompt or other non-user content accumulated in RequestMessages doesn't get minted
-            // into spurious entities/facts/preferences every turn.
+            // deliberately NOT persisted here (#89): ChatMessage.MessageId (used below for response-side
+            // dedup) is essentially never populated on caller-constructed request messages, so it can't
+            // help there -- request-message persistence ownership intentionally stays solely with
+            // Neo4jChatHistoryProvider. Building transient (never-persisted) Message objects for extraction
+            // only captures what the user said without risking a duplicate node. Filtered to ChatRole.User
+            // -- the same filter recall already applies (BuildContextAsync above) -- so a system prompt or
+            // other non-user content accumulated in RequestMessages doesn't get minted into spurious
+            // entities/facts/preferences every turn.
             var transientRequestMessages = requestMessages
                 .Where(msg => msg.Role == ChatRole.User && !string.IsNullOrWhiteSpace(msg.Text))
                 .Select(msg => MafTypeMapper.ToInternalMessage(msg, sessionId, conversationId, _clock, _idGenerator))
@@ -216,13 +215,27 @@ internal async Task PerformStoreAsync(
             {
                 if (string.IsNullOrWhiteSpace(msg.Text)) continue;
 
-                var stored = await _memoryService
-                    .AddMessageAsync(
-                        sessionId, conversationId,
-                        MafTypeMapper.ToInternalRole(msg.Role),
-                        msg.Text,
-                        cancellationToken: cancellationToken)
-                    .ConfigureAwait(false);
+                // When the underlying IChatClient stamps a provider-native MessageId on this response
+                // message, persist it under a deterministic id (#89): if another persisting component
+                // (e.g. Neo4jChatHistoryProvider, Neo4jChatMessageStore) configured on the same agent
+                // observes the same response, it converges on this same :Message node instead of creating
+                // a duplicate. Falls back to today's fresh-id behavior when absent (no regression).
+                var providerId = MafTypeMapper.TryGetProviderMessageId(msg);
+                var stored = providerId is not null
+                    ? await _memoryService
+                        .AddMessageWithIdAsync(
+                            sessionId, conversationId,
+                            MafTypeMapper.ToInternalRole(msg.Role),
+                            msg.Text, providerId,
+                            cancellationToken: cancellationToken)
+                        .ConfigureAwait(false)
+                    : await _memoryService
+                        .AddMessageAsync(
+                            sessionId, conversationId,
+                            MafTypeMapper.ToInternalRole(msg.Role),
+                            msg.Text,
+                            cancellationToken: cancellationToken)
+                        .ConfigureAwait(false);
                 storedMessages.Add(stored);
             }