.NET: Fix bug with per-service-call persistence and approvals (#4933)

* Fix bug with per-service-call persistence and approvals

* Apply suggestions from code review

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
westey
2026-03-26 17:45:46 +00:00
committed by GitHub
co-authored by Copilot
parent 63dee91a5f
commit 3585581c7a
8 changed files with 971 additions and 16 deletions
@@ -788,6 +788,13 @@ public sealed partial class ChatClientAgent : AIAgent
chatOptions.ConversationId = typedSession.ConversationId;
}
// When per-service-call persistence is active, set a sentinel conversation ID so that
// FunctionInvokingChatClient treats locally-persisted history the same as service-managed
// history. This prevents it from adding duplicate FunctionCallContent messages into the
// request when processing approval responses — the loaded history already contains them.
// ChatHistoryPersistingChatClient strips the sentinel before forwarding to the inner client.
chatOptions = this.SetLocalHistoryConversationIdIfNeeded(chatOptions);
// Materialize the accumulated messages once at the end of the provider pipeline, reusing the existing list if possible.
List<ChatMessage> messagesList = inputMessagesForChatClient as List<ChatMessage> ?? inputMessagesForChatClient.ToList();
@@ -929,6 +936,26 @@ public sealed partial class ChatClientAgent : AIAgent
}
}
/// <summary>
/// Sets the <see cref="ChatHistoryPersistingChatClient.LocalHistoryConversationId"/> sentinel on
/// <paramref name="chatOptions"/> when per-service-call persistence is active and no real
/// conversation ID is present.
/// </summary>
/// <returns>
/// The (possibly new) <see cref="ChatOptions"/> with the sentinel set, or the original
/// <paramref name="chatOptions"/> if no sentinel is needed.
/// </returns>
private ChatOptions? SetLocalHistoryConversationIdIfNeeded(ChatOptions? chatOptions)
{
if (this.PersistsChatHistoryPerServiceCall && string.IsNullOrWhiteSpace(chatOptions?.ConversationId))
{
chatOptions ??= new ChatOptions();
chatOptions.ConversationId = ChatHistoryPersistingChatClient.LocalHistoryConversationId;
}
return chatOptions;
}
/// <summary>
/// Gets a value indicating whether the agent has a <see cref="ChatHistoryPersistingChatClient"/>
/// decorator in mark-only mode, which marks messages for later persistence at the end of the run.
@@ -50,6 +50,26 @@ internal sealed class ChatHistoryPersistingChatClient : DelegatingChatClient
/// </summary>
internal const string PersistedMarkerKey = "_chatHistoryPersisted";
/// <summary>
/// A sentinel value set on <see cref="ChatOptions.ConversationId"/> by <see cref="ChatClientAgent"/>
/// when per-service-call persistence is active and no real conversation ID exists.
/// </summary>
/// <remarks>
/// <para>
/// This signals to <see cref="FunctionInvokingChatClient"/> that the chat history is being managed
/// externally (by this decorator), which prevents it from adding duplicate <see cref="FunctionCallContent"/>
/// messages into the request during approval-response processing. Without this sentinel,
/// <see cref="FunctionInvokingChatClient"/> would reconstruct function-call messages from approval
/// responses and append them to the original messages — but the loaded history already contains
/// those same function calls, causing duplicate tool-call entries that the model rejects.
/// </para>
/// <para>
/// This decorator strips the sentinel before forwarding requests to the inner client, so the
/// underlying model never sees it.
/// </para>
/// </remarks>
internal const string LocalHistoryConversationId = "_agent_local_history";
/// <summary>
/// Initializes a new instance of the <see cref="ChatHistoryPersistingChatClient"/> class.
/// </summary>
@@ -87,6 +107,7 @@ internal sealed class ChatHistoryPersistingChatClient : DelegatingChatClient
CancellationToken cancellationToken = default)
{
var (agent, session) = GetRequiredAgentAndSession();
options = StripLocalHistoryConversationId(options);
ChatResponse response;
try
@@ -130,6 +151,7 @@ internal sealed class ChatHistoryPersistingChatClient : DelegatingChatClient
[EnumeratorCancellation] CancellationToken cancellationToken = default)
{
var (agent, session) = GetRequiredAgentAndSession();
options = StripLocalHistoryConversationId(options);
List<ChatResponseUpdate> responseUpdates = [];
@@ -310,4 +332,20 @@ internal sealed class ChatHistoryPersistingChatClient : DelegatingChatClient
}
}
}
/// <summary>
/// If the <paramref name="options"/> carry the <see cref="LocalHistoryConversationId"/> sentinel,
/// returns a clone with the conversation ID cleared so the inner client never sees it.
/// Otherwise returns the original <paramref name="options"/> unchanged.
/// </summary>
private static ChatOptions? StripLocalHistoryConversationId(ChatOptions? options)
{
if (options?.ConversationId == LocalHistoryConversationId)
{
options = options.Clone();
options.ConversationId = null;
}
return options;
}
}