Merge pull request #110 from joslat/feature/persist-extract-full-turn-89

Persist and extract from the complete turn, not just the response (#89)

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 },
+            ]);
+        }
+    }
+}