.NET: Add helpers to more easily access in-memory ChatHistory and make ChatHistoryProvider management more configurable.#4224
Conversation
…HistoryProvider management more configurable.
There was a problem hiding this comment.
Pull request overview
This PR adds helper methods to access in-memory chat history from agent sessions and introduces configurable ChatHistoryProvider management options for ChatClientAgent, addressing issue #4140 where users could no longer easily access session messages in RC1.
Changes:
- Added
TryGetInMemoryChatHistoryandSetInMemoryChatHistoryextension methods onAgentSessionfor easier access to in-memory chat history - Introduced three new configuration options on
ChatClientAgentOptionsto control behavior when ChatHistoryProvider conflicts with server-side chat history:ThrowOnChatHistoryProviderConflict,WarnOnChatHistoryProviderConflict, andClearOnChatHistoryProviderConflict - Modified
ChatClientAgentconstructor to eagerly initializeChatHistoryProviderwithInMemoryChatHistoryProviderwhen none is provided, and updated conflict detection to distinguish between user-provided and default providers
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI.Abstractions/AgentSessionExtensions.cs | New extension methods providing convenient access to in-memory chat history state |
| dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgentOptions.cs | Added three boolean properties to control ChatHistoryProvider conflict handling behavior with appropriate defaults |
| dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgent.cs | Changed ChatHistoryProvider initialization from lazy to eager, updated conflict detection logic to check options instead of instance property |
| dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgentLogMessages.cs | Added warning log message for ChatHistoryProvider conflicts |
| dotnet/samples/GettingStarted/Agents/Agent_Step16_ChatReduction/Program.cs | Updated to use new TryGetInMemoryChatHistory extension method instead of accessing provider directly |
| dotnet/tests/Microsoft.Agents.AI.Abstractions.UnitTests/AgentSessionExtensionsTests.cs | Comprehensive tests for new extension methods covering all scenarios |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgent_ChatHistoryManagementTests.cs | New tests validating all conflict resolution behaviors |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgentOptionsTests.cs | Updated tests to verify new properties and their defaults |
dotnet/src/Microsoft.Agents.AI.Abstractions/AgentSessionExtensions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| /// <summary> | ||
| /// Gets or sets a value indicating whether to set the <see cref="ChatClientAgent.ChatHistoryProvider"/> to <see langword="null"/> | ||
| /// if the underlying AI service indicates that it manages chat history (for example, by returning a conversation id in the response), but a <see cref="ChatHistoryProvider"/> is configured for the agent. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Note that even if this setting is set to <see langword="false"/>, the <see cref="ChatHistoryProvider"/> will still not be used if the underlying AI service indicates that it manages chat history. | ||
| /// </remarks> | ||
| /// <value> | ||
| /// Default is <see langword="true"/>. | ||
| /// </value> | ||
| public bool ClearOnChatHistoryProviderConflict { get; set; } = true; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets a value indicating whether to log a warning if the underlying AI service indicates that it manages chat history | ||
| /// (for example, by returning a conversation id in the response), but a <see cref="ChatHistoryProvider"/> is configured for the agent. | ||
| /// </summary> | ||
| /// <value> | ||
| /// Default is <see langword="true"/>. | ||
| /// </value> | ||
| public bool WarnOnChatHistoryProviderConflict { get; set; } = true; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets a value indicating whether an exception is thrown if the underlying AI service indicates that it manages chat history | ||
| /// (for example, by returning a conversation id in the response), but a <see cref="ChatHistoryProvider"/> is configured for the agent. | ||
| /// </summary> | ||
| /// <value> | ||
| /// Default is <see langword="true"/>. | ||
| /// </value> | ||
| public bool ThrowOnChatHistoryProviderConflict { get; set; } = true; |
There was a problem hiding this comment.
Since those properties can be together in the same at the same time, I think we can use a flags approach.
What the current 3 properties do (in practice)
These three booleans model how to react when there’s a configuration conflict:
- The AI service says “I manage chat history” (e.g., returns a conversation id), but
- the agent has a
ChatHistoryProviderconfigured.
The options then control three independent reactions:
- Clear the agent’s
ChatHistoryProvider(ClearOn...) - Warn (log a warning) (
WarnOn...) - Throw (raise an exception) (
ThrowOn...)
Suggestion: replace with one flags property
Define an enum like:
[Flags]
public enum ChatHistoryProviderConflictBehavior
{
None = 0,
Clear = 1 << 0,
Warn = 1 << 1,
Throw = 1 << 2,
}Then in ChatClientAgentOptions:
public ChatHistoryProviderConflictBehavior ChatHistoryProviderConflictBehavior { get; set; }
= ChatHistoryProviderConflictBehavior.Clear
| ChatHistoryProviderConflictBehavior.Warn
| ChatHistoryProviderConflictBehavior.Throw;Usage:
options.ChatHistoryProviderConflictBehavior =
ChatHistoryProviderConflictBehavior.Warn | ChatHistoryProviderConflictBehavior.Clear;How you check it in code
Wherever you currently do if (WarnOn...), etc.:
var behavior = options.ChatHistoryProviderConflictBehavior;
if (behavior.HasFlag(ChatHistoryProviderConflictBehavior.Throw))
{
throw new InvalidOperationException(...);
}
if (behavior.HasFlag(ChatHistoryProviderConflictBehavior.Warn))
{
logger.LogWarning(...);
}
if (behavior.HasFlag(ChatHistoryProviderConflictBehavior.Clear))
{
agent.ChatHistoryProvider = null;
}Does it make sense?
There was a problem hiding this comment.
Does it make sense?
Yes, an enum is generally more elegant, but enums do have other issues. If we wanted to add an additional flag, it's a breaking change with an enum, whereas adding a new bool to options is not. That's why I chose bools rather than an enum.
Motivation and Context
#4140
Description
Contribution Checklist