diff --git a/dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs b/dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs index 9e79ac5b780..85a6aad57a3 100644 --- a/dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs +++ b/dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs @@ -478,7 +478,7 @@ internal async Task NotifyProvidersOfNewMessagesAsync( ChatOptions? chatOptions, CancellationToken cancellationToken) { - ChatHistoryProvider? chatHistoryProvider = this.ResolveChatHistoryProvider(chatOptions); + ChatHistoryProvider? chatHistoryProvider = this.ResolveChatHistoryProvider(session, chatOptions); if (chatHistoryProvider is not null) { @@ -510,7 +510,7 @@ internal async Task NotifyProvidersOfFailureAsync( ChatOptions? chatOptions, CancellationToken cancellationToken) { - ChatHistoryProvider? chatHistoryProvider = this.ResolveChatHistoryProvider(chatOptions); + ChatHistoryProvider? chatHistoryProvider = this.ResolveChatHistoryProvider(session, chatOptions); if (chatHistoryProvider is not null) { @@ -980,22 +980,33 @@ private void WarnOnMissingPerServiceCallChatHistoryPersistingChatClient() } } - private ChatHistoryProvider? ResolveChatHistoryProvider(ChatOptions? chatOptions) + private ChatHistoryProvider? ResolveChatHistoryProvider(ChatClientAgentSession session, ChatOptions? chatOptions) { - ChatHistoryProvider? provider = - chatOptions?.ConversationId is null || IsAGUIProviderName(this._agentMetadata.ProviderName) - ? this.ChatHistoryProvider - : null; + // A service that manages chat history server-side disengages the chat history provider so that history + // is not stored in two places. The service is considered to store history when a conversation id is + // present either on the options (explicitly supplied by the caller) or on the session (returned by the + // service on a previous or the current run). + // + // The per-service-call persistence check must remain: PerServiceCallChatHistoryPersistingChatClient + // calls back into LoadChatHistoryAsync/NotifyProviders (which reach here) precisely when per-service-call + // persistence is active, and in its simulated path it stamps a sentinel onto session.ConversationId. + // Without this check that sentinel would be mistaken for service-stored history and wrongly disengage + // the provider the decorator depends on. + bool serviceStoresHistory = + !this.RequiresPerServiceCallChatHistoryPersistence + && !IsAGUIProviderName(this._agentMetadata.ProviderName) + && (!string.IsNullOrWhiteSpace(chatOptions?.ConversationId) + || !string.IsNullOrWhiteSpace(session.ConversationId)); + + ChatHistoryProvider? provider = serviceStoresHistory ? null : this.ChatHistoryProvider; // If someone provided an override ChatHistoryProvider via AdditionalProperties, we should use that instead. if (chatOptions?.AdditionalProperties?.TryGetValue(out ChatHistoryProvider? overrideProvider) is true) { - if (!IsAGUIProviderName(this._agentMetadata.ProviderName) && - this._agentOptions?.ThrowOnChatHistoryProviderConflict is true && - string.IsNullOrWhiteSpace(chatOptions?.ConversationId) is false) + if (this._agentOptions?.ThrowOnChatHistoryProviderConflict is true && serviceStoresHistory) { throw new InvalidOperationException( - $"Only {nameof(ChatClientAgentSession.ConversationId)} or {nameof(this.ChatHistoryProvider)} may be used, but not both. The current {nameof(ChatClientAgentSession)} has a {nameof(ChatClientAgentSession.ConversationId)} indicating server-side chat history management, but an override {nameof(this.ChatHistoryProvider)} was provided via {nameof(AgentRunOptions.AdditionalProperties)}."); + $"Only {nameof(ChatClientAgentSession.ConversationId)} or {nameof(this.ChatHistoryProvider)} may be used, but not both. A {nameof(ChatClientAgentSession.ConversationId)} indicating server-side chat history management is present (on the {nameof(ChatClientAgentSession)} or the {nameof(this.ChatOptions)}), but an override {nameof(this.ChatHistoryProvider)} was provided via {nameof(AgentRunOptions.AdditionalProperties)}."); } // Validate that the override provider's StateKeys do not clash with any AIContextProvider's StateKeys. @@ -1030,7 +1041,7 @@ internal async Task> LoadChatHistoryAsync( ChatOptions? chatOptions, CancellationToken cancellationToken) { - var chatHistoryProvider = this.ResolveChatHistoryProvider(chatOptions); + var chatHistoryProvider = this.ResolveChatHistoryProvider(session, chatOptions); if (chatHistoryProvider is null) { return messages; diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs index 410ee4edda1..356a41511ea 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs @@ -411,6 +411,150 @@ public async Task RunAsync_DoesNotThrow_WhenNoChatHistoryProviderInOptionsAndCon Assert.Equal("ConvId", session!.ConversationId); } + /// + /// Regression test for https://github.com/microsoft/agent-framework/issues/6120. + /// When the service manages chat history server-side (returns a conversation id), the framework's + /// default in-memory chat history provider must not persist the messages, even on the first turn. + /// + [Fact] + public async Task RunAsync_DoesNotUseDefaultInMemoryChatHistoryProvider_WhenConversationIdReturnedAsync() + { + // Arrange + Mock mockService = new(); + mockService.Setup( + s => s.GetResponseAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny())).ReturnsAsync(new ChatResponse([new(ChatRole.Assistant, "response")]) { ConversationId = "ConvId" }); + ChatClientAgent agent = new(mockService.Object, options: new() + { + ChatOptions = new() { Instructions = "test instructions" }, + }); + + // Act + ChatClientAgentSession? session = await agent.CreateSessionAsync() as ChatClientAgentSession; + await agent.RunAsync([new(ChatRole.User, "test")], session); + + // Assert + Assert.Equal("ConvId", session!.ConversationId); + var inMemoryProvider = Assert.IsType(agent.ChatHistoryProvider); + Assert.Empty(inMemoryProvider.GetMessages(session)); + } + + /// + /// Regression test for https://github.com/microsoft/agent-framework/issues/6120. + /// The streaming path must also refrain from populating the default in-memory chat history provider + /// when the service returns a conversation id. + /// + [Fact] + public async Task RunStreamingAsync_DoesNotUseDefaultInMemoryChatHistoryProvider_WhenConversationIdReturnedAsync() + { + // Arrange + ChatResponseUpdate[] returnUpdates = + [ + new ChatResponseUpdate(role: ChatRole.Assistant, content: "response") { ConversationId = "ConvId" }, + ]; + Mock mockService = new(); + mockService.Setup( + s => s.GetStreamingResponseAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny())).Returns(returnUpdates.ToAsyncEnumerable()); + ChatClientAgent agent = new(mockService.Object, options: new() + { + ChatOptions = new() { Instructions = "test instructions" }, + }); + + // Act + ChatClientAgentSession? session = await agent.CreateSessionAsync() as ChatClientAgentSession; + await foreach (var _ in agent.RunStreamingAsync([new(ChatRole.User, "test")], session)) + { + } + + // Assert + Assert.Equal("ConvId", session!.ConversationId); + var inMemoryProvider = Assert.IsType(agent.ChatHistoryProvider); + Assert.Empty(inMemoryProvider.GetMessages(session)); + } + + /// + /// Regression test for https://github.com/microsoft/agent-framework/issues/6120. + /// Across multiple turns backed by service-stored history, the default in-memory chat history provider + /// is never populated and prior turns are not replayed to the service (the service owns the history). + /// + [Fact] + public async Task RunAsync_MultiTurnServiceStoredHistory_DoesNotPopulateDefaultInMemoryProviderAsync() + { + // Arrange + var capturedInputs = new List>(); + Mock mockService = new(); + mockService.Setup( + s => s.GetResponseAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny())) + .Returns, ChatOptions?, CancellationToken>((msgs, _, _) => + { + capturedInputs.Add(msgs.ToList()); + return Task.FromResult(new ChatResponse([new(ChatRole.Assistant, "response")]) { ConversationId = "ConvId" }); + }); + ChatClientAgent agent = new(mockService.Object, options: new() + { + ChatOptions = new() { Instructions = "test instructions" }, + }); + + // Act + ChatClientAgentSession? session = await agent.CreateSessionAsync() as ChatClientAgentSession; + await agent.RunAsync([new(ChatRole.User, "first")], session); + await agent.RunAsync([new(ChatRole.User, "second")], session); + + // Assert + Assert.Equal("ConvId", session!.ConversationId); + var inMemoryProvider = Assert.IsType(agent.ChatHistoryProvider); + Assert.Empty(inMemoryProvider.GetMessages(session)); + + // The second turn should only send the new user message, since the service owns the history. + Assert.Equal(2, capturedInputs.Count); + Assert.Single(capturedInputs[1]); + Assert.Equal("second", capturedInputs[1][0].Text); + } + + /// + /// When the service manages chat history server-side (returns a conversation id), an explicitly-configured + /// chat history provider is disengaged just like the default provider, even when all conflict handling is + /// disabled. This pins the uniform "service storage disengages any provider" semantics. + /// + [Fact] + public async Task RunAsync_ExplicitChatHistoryProvider_Disengaged_WhenConflictHandlingDisabledAndConversationIdReturnedAsync() + { + // Arrange + Mock mockService = new(); + mockService.Setup( + s => s.GetResponseAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny())).ReturnsAsync(new ChatResponse([new(ChatRole.Assistant, "response")]) { ConversationId = "ConvId" }); + var chatHistoryProvider = new InMemoryChatHistoryProvider(); + ChatClientAgent agent = new(mockService.Object, options: new() + { + ChatOptions = new() { Instructions = "test instructions" }, + ChatHistoryProvider = chatHistoryProvider, + ThrowOnChatHistoryProviderConflict = false, + ClearOnChatHistoryProviderConflict = false, + WarnOnChatHistoryProviderConflict = false, + }); + + // Act + ChatClientAgentSession? session = await agent.CreateSessionAsync() as ChatClientAgentSession; + await agent.RunAsync([new(ChatRole.User, "test")], session); + + // Assert — the provider reference is retained (conflict handling disabled), but it is not persisted to + // because the service stores history. + Assert.Equal("ConvId", session!.ConversationId); + Assert.Same(chatHistoryProvider, agent.ChatHistoryProvider); + Assert.Empty(chatHistoryProvider.GetMessages(session)); + } + #endregion #region ChatHistoryProvider Override Tests