fix: persist and extract from the complete turn, not just the response (#89)

Neo4jMemoryContextProvider.StoreAIContextAsync/PerformStoreAsync and
Neo4jChatHistoryProvider.StoreChatHistoryAsync only ran extraction over
the assistant's response messages -- a turn like "User: I prefer window
seats. / Assistant: Got it." only extracted from "Got it.", so a
user-stated preference was invisible to extraction unless the assistant
happened to repeat it.

Per explicit scoping: extraction must see request + response, but
message persistence must not blindly duplicate what another configured
component (a chat-history provider/store) may already be persisting.
Research confirmed there is no idempotency mechanism anywhere in the
stack today (Neo4jMessageRepository.AddAsync always CREATEs a new node;
MemoryService.AddMessageAsync always generates a fresh id; there is no
caller-supplied-id or MERGE-by-id path) -- so naively concatenating and
persisting everything would risk duplicate :Message nodes whenever a
host also has a chat-history component configured.

Two different fix shapes, split by what each provider already persists:
- Neo4jChatHistoryProvider already persisted both request and response
  messages; the fix just also feeds the request messages to extraction
  (previously discarded after being persisted). Zero new duplication
  risk. Refactored StoreChatHistoryAsync's body into an internal
  PerformStoreAsync test seam, mirroring the sibling provider.
- Neo4jMemoryContextProvider never persisted request messages; the fix
  deliberately keeps it that way (avoiding the duplication risk) and
  instead builds transient, never-persisted Message objects purely for
  extraction. The trade-off -- extracted facts/preferences from the
  user's request are captured correctly, but their EXTRACTED_FROM
  provenance edge and source_message_ids won't resolve to a real node
  unless another component also persisted that exact message -- is
  documented, not silently accepted.

An adversarial review confirmed the no-duplication guarantee holds (no
path from ExtractAndPersistAsync ever calls AddMessageAsync) and found
one real gap worth fixing: request messages were being fed to extraction
without the same ChatRole.User filter recall already applies, so a
system prompt accumulated in RequestMessages could get minted into
spurious entities/facts/preferences every turn. Fixed in both providers.
It also noted (out of this issue's scope, pre-existing, not
introduced here) that response-message persistence itself has no
idempotency guarantee either -- documented as a known limitation when
combining more than one message-persisting component on the same agent.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: joslat

docs/agent-framework.md

@@ -48,7 +48,7 @@ derives from `Microsoft.Agents.AI.AIContextProvider` and implements the framewor
 | MAF hook (C#) | When | What AgentMemory does | Python equivalent |
 |---|---|---|---|
 | `ProvideAIContextAsync(InvokingContext)` → `AIContext` | **Before** the model call | Embeds the user turn, recalls relevant memory, and returns it as `AIContext.Messages` to prepend to the prompt. | `before_run` |
-| `StoreAIContextAsync(InvokedContext)` | **After** the response | Persists the response messages and (optionally) runs extraction into the graph. Skipped if the run threw (`context.InvokeException`). | `after_run` |
+| `StoreAIContextAsync(InvokedContext)` | **After** the response | Persists the response messages and (optionally) runs extraction over the complete turn (request + response) into the graph. Skipped if the run threw (`context.InvokeException`). | `after_run` |
 
 This is the same **bidirectional** behavior the official provider describes ("auto-retrieve before
 invocation, auto-save after responses") — recall is passive and automatic; you never call it by hand.
@@ -58,7 +58,28 @@ Native recall via `Neo4jMemoryContextProvider` respects your configured `MemoryO
 `IMemoryService.RecallAsync(...)` call (#87). The one exception is `RecallOptions.Scope`: native recall
 always derives scope from the invocation's authenticated owner (via #100's isolation policy), never from a
 statically configured `Scope`, so a global config value can't silently override the real, per-invocation
-owner. `Neo4jMicrosoftMemoryFacade` (the lower-level, manually-driven alternative to the context provider)
+owner. Automatic extraction (`AutoExtractOnPersist`) considers the **complete turn** — both what the user asked
+and what the assistant answered (#89), filtered to user-role content so a system prompt or other non-user
+text isn't minted into spurious entities/facts/preferences every turn — so a preference or fact the user
+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.
+
+`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
 covered by this fix.
 

src/AgentMemory.AgentFramework/Neo4jChatHistoryProvider.cs

@@ -104,26 +104,45 @@ protected override async ValueTask StoreChatHistoryAsync(
     {
         var (sessionId, conversationId, userId, applicationId) = ExtractIds(context.Session, context.Agent);
         using var storeScope = ApplyStoreContext(applicationId);
+        await PerformStoreAsync(
+            context.RequestMessages, context.ResponseMessages ?? [], sessionId, conversationId, cancellationToken, userId)
+            .ConfigureAwait(false);
+    }
+
+    /// <summary>Internal helper exposed for unit testing.</summary>
+    internal async Task PerformStoreAsync(
+        IEnumerable<ChatMessage> requestMessages,
+        IEnumerable<ChatMessage> responseMessages,
+        string sessionId,
+        string conversationId,
+        CancellationToken cancellationToken,
+        string? userId = null)
+    {
         using var ownerScope = ApplyOwnerContext(userId);
         try
         {
-            // Persist request messages (user + system turns not already in memory)
-            foreach (var msg in context.RequestMessages)
+            // Persist request messages (user + system turns not already in memory). Both request and
+            // response messages are genuinely persisted here, so -- unlike Neo4jMemoryContextProvider,
+            // which deliberately does NOT persist request messages to avoid duplicating what this
+            // provider already stores -- extraction can safely see both with full provenance (#89).
+            var storedRequests = new List<Abstractions.Domain.Message>();
+            foreach (var msg in requestMessages)
             {
                 if (string.IsNullOrWhiteSpace(msg.Text)) continue;
                 var message = MafTypeMapper.ToInternalMessage(
                     msg, sessionId, conversationId, _clock, _idGenerator);
-                await _memoryService
+                var stored = await _memoryService
                     .AddMessageAsync(
                         message.SessionId, message.ConversationId,
                         message.Role, message.Content, message.Metadata,
                         cancellationToken)
                     .ConfigureAwait(false);
+                storedRequests.Add(stored);
             }
 
             // Persist response messages
             var storedResponses = new List<Abstractions.Domain.Message>();
-            foreach (var msg in context.ResponseMessages ?? [])
+            foreach (var msg in responseMessages)
             {
                 if (string.IsNullOrWhiteSpace(msg.Text)) continue;
                 var stored = await _memoryService
@@ -136,15 +155,20 @@ await _memoryService
                 storedResponses.Add(stored);
             }
 
-            // Optionally trigger knowledge extraction on the persisted responses
-            if (_options.AutoExtractOnPersist && storedResponses.Count > 0)
+            // Optionally trigger knowledge extraction on the complete persisted turn (#89): a fact or
+            // preference the user states in their request is now captured even if the assistant never
+            // repeats it back. Extraction input is filtered to user-role request messages -- persistence
+            // above still stores every role (unchanged), but a system prompt or other non-user content
+            // in RequestMessages shouldn't be minted into spurious entities/facts/preferences every turn.
+            var turnMessages = storedRequests.Where(m => m.Role == "user").Concat(storedResponses).ToList();
+            if (_options.AutoExtractOnPersist && turnMessages.Count > 0)
             {
                 try
                 {
                     await _memoryService.ExtractAndPersistAsync(
                         new Abstractions.Domain.ExtractionRequest
                         {
-                            Messages = storedResponses,
+                            Messages = turnMessages,
                             SessionId = sessionId,
                             UserId = userId
                         }, cancellationToken).ConfigureAwait(false);

src/AgentMemory.AgentFramework/Neo4jMemoryContextProvider.cs

@@ -16,6 +16,8 @@ public sealed class Neo4jMemoryContextProvider : AIContextProvider
 {
     private readonly IMemoryService _memoryService;
     private readonly IEmbeddingOrchestrator _embeddingOrchestrator;
+    private readonly IClock _clock;
+    private readonly IIdGenerator _idGenerator;
     private readonly RecallOptions _recallOptions;
     private readonly ContextFormatOptions _formatOptions;
     private readonly AgentFrameworkOptions _agentOptions;
@@ -26,6 +28,8 @@ public sealed class Neo4jMemoryContextProvider : AIContextProvider
     public Neo4jMemoryContextProvider(
         IMemoryService memoryService,
         IEmbeddingOrchestrator embeddingOrchestrator,
+        IClock clock,
+        IIdGenerator idGenerator,
         IOptions<MemoryOptions> memoryOptions,
         IOptions<ContextFormatOptions> formatOptions,
         IOptions<AgentFrameworkOptions> agentOptions,
@@ -39,6 +43,8 @@ public Neo4jMemoryContextProvider(
     {
         _memoryService = memoryService ?? throw new ArgumentNullException(nameof(memoryService));
         _embeddingOrchestrator = embeddingOrchestrator ?? throw new ArgumentNullException(nameof(embeddingOrchestrator));
+        _clock = clock ?? throw new ArgumentNullException(nameof(clock));
+        _idGenerator = idGenerator ?? throw new ArgumentNullException(nameof(idGenerator));
         _recallOptions = memoryOptions?.Value.Recall ?? RecallOptions.Default;
         _formatOptions = formatOptions?.Value ?? new ContextFormatOptions();
         _agentOptions = agentOptions?.Value ?? new AgentFrameworkOptions();
@@ -169,16 +175,18 @@ protected override async ValueTask StoreAIContextAsync(
             return;
         }
 
+        var requestMessages = context.RequestMessages ?? Enumerable.Empty<ChatMessage>();
         var responseMessages = context.ResponseMessages ?? Enumerable.Empty<ChatMessage>();
         var ids = ExtractIds(context.Session, context.Agent);
         using var storeScope = ApplyStoreContext(ids.applicationId);
 
-        await PerformStoreAsync(responseMessages, ids.sessionId, ids.conversationId, cancellationToken, ids.userId)
+        await PerformStoreAsync(requestMessages, responseMessages, ids.sessionId, ids.conversationId, cancellationToken, ids.userId)
             .ConfigureAwait(false);
     }
 
     /// <summary>Internal helper exposed for unit testing.</summary>
     internal async Task PerformStoreAsync(
+        IEnumerable<ChatMessage> requestMessages,
         IEnumerable<ChatMessage> responseMessages,
         string sessionId,
         string conversationId,
@@ -188,6 +196,21 @@ internal async Task PerformStoreAsync(
         using var ownerScope = _ownerContext?.BeginOwnerScope(userId);
         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.
+            var transientRequestMessages = requestMessages
+                .Where(msg => msg.Role == ChatRole.User && !string.IsNullOrWhiteSpace(msg.Text))
+                .Select(msg => MafTypeMapper.ToInternalMessage(msg, sessionId, conversationId, _clock, _idGenerator))
+                .ToList();
+
             var storedMessages = new List<Message>();
             foreach (var msg in responseMessages)
             {
@@ -203,14 +226,25 @@ internal async Task PerformStoreAsync(
                 storedMessages.Add(stored);
             }
 
-            if (_agentOptions.AutoExtractOnPersist && storedMessages.Count > 0)
+            // Extraction sees the complete turn (#89): a fact or preference the user states in their
+            // request is now captured even if the assistant never repeats it back. Because
+            // transientRequestMessages were never persisted as :Message nodes above, provenance for
+            // anything extracted from them is incomplete: the EXTRACTED_FROM edge silently fails to
+            // attach (the MATCH in PersistenceStage's linking Cypher finds no such node), AND the
+            // extracted node's own source_message_ids property will still list the transient (never
+            // persisted) message id, i.e. a dangling reference, not just a missing edge. The extracted
+            // fact/preference itself is still created and recallable correctly; only its link back to the
+            // literal source message is best-effort, not guaranteed, unless another component also
+            // persisted that exact message.
+            var turnMessages = transientRequestMessages.Concat(storedMessages).ToList();
+            if (_agentOptions.AutoExtractOnPersist && turnMessages.Count > 0)
             {
                 try
                 {
                     await _memoryService.ExtractAndPersistAsync(
                         new ExtractionRequest
                         {
-                            Messages = storedMessages,
+                            Messages = turnMessages,
                             SessionId = sessionId,
                             UserId = userId
                         }, cancellationToken).ConfigureAwait(false);

tests/AgentMemory.Tests.Integration/AgentFramework/PersistExtractFullTurnIntegrationTests.cs

@@ -0,0 +1,119 @@
+using Microsoft.Extensions.AI;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using FluentAssertions;
+using AgentMemory;
+using AgentMemory.Abstractions.Domain;
+using AgentMemory.Abstractions.Services;
+using AgentMemory.AgentFramework;
+using AgentMemory.Core.Stubs;
+using AgentMemory.Tests.Integration.Fixtures;
+
+namespace AgentMemory.Tests.Integration.AgentFramework;
+
+/// <summary>
+/// Live-Neo4j proof of #89's core acceptance criterion: a preference stated ONLY in the user's request
+/// (never repeated by the assistant) is extracted and later recallable in a brand-new session, using a
+/// real, DI-resolved <see cref="Neo4jMemoryContextProvider"/> against real Neo4j.
+/// </summary>
+[Collection("Neo4j Integration")]
+[Trait("Category", "Integration")]
+public sealed class PersistExtractFullTurnIntegrationTests : IAsyncLifetime
+{
+    private readonly Neo4jIntegrationFixture _fixture;
+    private ServiceProvider _provider = null!;
+
+    public PersistExtractFullTurnIntegrationTests(Neo4jIntegrationFixture fixture) => _fixture = fixture;
+
+    public async Task InitializeAsync()
+    {
+        await _fixture.CleanDatabaseAsync();
+
+        var services = new ServiceCollection();
+        services.AddLogging();
+
+        // Deterministic preference extractor registered BEFORE AddNeo4jAgentMemory so it wins the
+        // TryAddScoped override -- same convention as ExtractionOwnerStampIntegrationTests.cs.
+        services.AddSingleton<IPreferenceExtractor, DeterministicPreferenceExtractor>();
+
+        services.AddNeo4jAgentMemory(
+            configureMemory: _ => { },
+            configureNeo4j: o =>
+            {
+                o.Uri = _fixture.ConnectionString;
+                o.Username = _fixture.User;
+                o.Password = _fixture.Password;
+                o.Database = "neo4j";
+                o.EmbeddingDimensions = Neo4jIntegrationFixture.TestEmbeddingDimensions;
+            });
+        services.AddAgentMemoryFramework(o => o.AutoExtractOnPersist = true);
+        services.AddSingleton<IEmbeddingGenerator<string, Embedding<float>>>(sp =>
+            new StubEmbeddingGenerator(
+                sp.GetRequiredService<ILogger<StubEmbeddingGenerator>>(),
+                Neo4jIntegrationFixture.TestEmbeddingDimensions));
+
+        _provider = services.BuildServiceProvider(validateScopes: true);
+    }
+
+    public async Task DisposeAsync()
+    {
+        if (_provider is not null) await _provider.DisposeAsync();
+    }
+
+    [Fact]
+    public async Task PreferenceStatedOnlyByUser_IsExtractedAndRecallableInANewSession()
+    {
+        // StubEmbeddingGenerator is deterministic (same text -> same vector); recall is a similarity
+        // search (MemoryContextAssembler -> SearchPreferencesAsync), so the recall query below
+        // deliberately reuses this exact marker text to guarantee a match, matching the
+        // "identical embeddings" convention used elsewhere in this test suite
+        // (e.g. McpResourceIsolationIntegrationTests' ContextResource tests).
+        const string preferenceMarker = "I prefer dark mode everywhere.";
+
+        using var writeScope = _provider.CreateScope();
+        var writeSp = writeScope.ServiceProvider;
+        var memoryProvider = writeSp.GetRequiredService<Neo4jMemoryContextProvider>();
+
+        // The assistant's reply never repeats the preference -- extraction must still capture it because
+        // it now sees the request message too (#89), not just the response.
+        await memoryProvider.PerformStoreAsync(
+            requestMessages: [new ChatMessage(ChatRole.User, preferenceMarker)],
+            responseMessages: [new ChatMessage(ChatRole.Assistant, "Got it.")],
+            sessionId: "session-full-turn",
+            conversationId: "conv-full-turn",
+            cancellationToken: CancellationToken.None,
+            userId: "alice");
+
+        // Recall in a brand-new session for the same owner.
+        using var recallScope = _provider.CreateScope();
+        var recallSp = recallScope.ServiceProvider;
+        var recallProvider = recallSp.GetRequiredService<Neo4jMemoryContextProvider>();
+
+        var context = await recallProvider.BuildContextAsync(
+            [new ChatMessage(ChatRole.User, preferenceMarker)],
+            sessionId: "session-new",
+            conversationId: "conv-new",
+            cancellationToken: CancellationToken.None,
+            userId: "alice");
+
+        context.Messages.Should().NotBeNullOrEmpty();
+        context.Messages!.Should().Contain(m => m.Text != null && m.Text.Contains("dark mode"),
+            "a preference stated only in the user's request must be extracted and recallable in a later session");
+    }
+
+    private sealed class DeterministicPreferenceExtractor : IPreferenceExtractor
+    {
+        public Task<IReadOnlyList<ExtractedPreference>> ExtractAsync(
+            IReadOnlyList<Message> messages, CancellationToken cancellationToken = default)
+        {
+            var userMessage = messages.FirstOrDefault(m => m.Role == "user");
+            if (userMessage is null || !userMessage.Content.Contains("prefer", StringComparison.OrdinalIgnoreCase))
+                return Task.FromResult<IReadOnlyList<ExtractedPreference>>(Array.Empty<ExtractedPreference>());
+
+            return Task.FromResult<IReadOnlyList<ExtractedPreference>>(
+            [
+                new ExtractedPreference { Category = "style", PreferenceText = userMessage.Content, Confidence = 0.95 },
+            ]);
+        }
+    }
+}