.NET: Fixes for ChatClientAgent to improve MEAI static analysis compliance (#502)

* Fixes for ChatClientAgent to improve MEAI static analysis compliance

* Improve instruction handling and fix unit tests.

* Address PR comments.

dotnet/src/Microsoft.Extensions.AI.Agents.AzureAI/NewPersistentAgentsChatClient.cs

@@ -256,6 +256,7 @@ namespace Azure.AI.Agents.Persistent
             // Populate the run options from the ChatOptions, if provided.
             if (options is not null)
             {
+                runOptions.OverrideInstructions ??= options.Instructions ?? _agent.Instructions;
                 runOptions.MaxCompletionTokens ??= options.MaxOutputTokens;
                 runOptions.OverrideModelName ??= options.ModelId;
                 runOptions.TopP ??= options.TopP;

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/AgentChatClientBuilderExtensions.cs

@@ -5,13 +5,13 @@ using Microsoft.Shared.Diagnostics;
 
 namespace Microsoft.Extensions.AI.Agents;
 
-/// <summary>Provides extensions for configuring <see cref="AgentInvokingChatClient"/> instances.</summary>
+/// <summary>Provides extensions for configuring <see cref="AgentInvokedChatClient"/> instances.</summary>
 public static class AgentChatClientBuilderExtensions
 {
     /// <summary>
     /// Enables automatic function call invocation on the chat pipeline.
     /// </summary>
-    /// <remarks>This works by adding an instance of <see cref="AgentInvokingChatClient"/> with default options.</remarks>
+    /// <remarks>This works by adding an instance of <see cref="AgentInvokedChatClient"/> with default options.</remarks>
     /// <param name="builder">The <see cref="ChatClientBuilder"/> being used to build the chat pipeline.</param>
     /// <returns>The supplied <paramref name="builder"/>.</returns>
     /// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
@@ -22,7 +22,7 @@ public static class AgentChatClientBuilderExtensions
 
         return builder.Use((innerClient, services) =>
         {
-            return new AgentInvokingChatClient(innerClient);
+            return new AgentInvokedChatClient(innerClient);
         });
     }
 }

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/AgentInvokedChatClient.cs

@@ -5,13 +5,13 @@ namespace Microsoft.Extensions.AI.Agents;
 /// <summary>
 /// Internal chat client that handle agent invocation details for the chat client pipeline.
 /// </summary>
-internal sealed class AgentInvokingChatClient : DelegatingChatClient
+internal sealed class AgentInvokedChatClient : DelegatingChatClient
 {
     /// <summary>
-    /// Initializes a new instance of the <see cref="AgentInvokingChatClient"/> class.
+    /// Initializes a new instance of the <see cref="AgentInvokedChatClient"/> class.
     /// </summary>
     /// <param name="chatClient">The chat client to invoke agents.</param>
-    internal AgentInvokingChatClient(IChatClient chatClient)
+    internal AgentInvokedChatClient(IChatClient chatClient)
         : base(chatClient)
     {
     }

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/ChatClientAgent.cs

@@ -9,6 +9,8 @@ using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Shared.Diagnostics;
 
+#pragma warning disable S3358 // Ternary operators should not be nested
+
 namespace Microsoft.Extensions.AI.Agents;
 
 /// <summary>
@@ -33,12 +35,12 @@ public sealed class ChatClientAgent : AIAgent
     public ChatClientAgent(IChatClient chatClient, string? instructions = null, string? name = null, string? description = null, IList<AITool>? tools = null, ILoggerFactory? loggerFactory = null)
         : this(
               chatClient,
-              new ChatClientAgentOptions()
+              new ChatClientAgentOptions
               {
                   Name = name,
                   Description = description,
                   Instructions = instructions,
-                  ChatOptions = tools is null ? null : new ChatOptions()
+                  ChatOptions = tools is null ? null : new ChatOptions
                   {
                       Tools = tools,
                   }
@@ -55,7 +57,7 @@ public sealed class ChatClientAgent : AIAgent
     /// <param name="loggerFactory">Optional logger factory to use for logging.</param>
     public ChatClientAgent(IChatClient chatClient, ChatClientAgentOptions options, ILoggerFactory? loggerFactory = null)
     {
-        Throw.IfNull(chatClient);
+        _ = Throw.IfNull(chatClient);
 
         // Options must be cloned since ChatClientAgentOptions is mutable.
         this._agentOptions = options?.Clone();
@@ -65,13 +67,14 @@ public sealed class ChatClientAgent : AIAgent
         // Get the type of the chat client before wrapping it as an agent invoking chat client.
         this._chatClientType = chatClient.GetType();
 
-        this.ChatClient = chatClient.AsAgentInvokingChatClient();
+        // If the user has not opted out of using our default decorators, we wrap the chat client.
+        this.ChatClient = options?.UseProvidedChatClientAsIs is true ? chatClient : chatClient.AsAgentInvokedChatClient();
 
         this._logger = (loggerFactory ?? chatClient.GetService<ILoggerFactory>() ?? NullLoggerFactory.Instance).CreateLogger<ChatClientAgent>();
     }
 
     /// <summary>
-    /// The underlying chat client used by the agent to invoke chat completions.
+    /// Gets the underlying chat client used by the agent to invoke chat completions.
     /// </summary>
     public IChatClient ChatClient { get; }
 
@@ -101,7 +104,7 @@ public sealed class ChatClientAgent : AIAgent
         AgentRunOptions? options = null,
         CancellationToken cancellationToken = default)
     {
-        Throw.IfNull(messages);
+        _ = Throw.IfNull(messages);
 
         (AgentThread safeThread, ChatOptions? chatOptions, List<ChatMessage> threadMessages) =
             await this.PrepareThreadAndMessagesAsync(thread, messages, options, cancellationToken).ConfigureAwait(false);
@@ -190,15 +193,17 @@ public sealed class ChatClientAgent : AIAgent
 
     /// <inheritdoc/>
     public override object? GetService(Type serviceType, object? serviceKey = null)
-        => base.GetService(serviceType, serviceKey)
-            ?? (serviceType == typeof(AIAgentMetadata) ? this._agentMetadata
-            : serviceType == typeof(IChatClient) ? this.ChatClient
-            : this.ChatClient.GetService(serviceType, serviceKey));
+    {
+        return base.GetService(serviceType, serviceKey)
+        ?? (serviceType == typeof(AIAgentMetadata) ? this._agentMetadata
+        : serviceType == typeof(IChatClient) ? this.ChatClient
+        : this.ChatClient.GetService(serviceType, serviceKey));
+    }
 
     /// <inheritdoc/>
     public override AgentThread GetNewThread()
     {
-        var thread = new AgentThread() { MessageStore = this._agentOptions?.ChatMessageStoreFactory?.Invoke() };
+        var thread = new AgentThread { MessageStore = this._agentOptions?.ChatMessageStoreFactory?.Invoke() };
         return thread;
     }
 
@@ -235,6 +240,7 @@ public sealed class ChatClientAgent : AIAgent
         requestChatOptions.AllowMultipleToolCalls ??= this._agentOptions.ChatOptions.AllowMultipleToolCalls;
         requestChatOptions.ConversationId ??= this._agentOptions.ChatOptions.ConversationId;
         requestChatOptions.FrequencyPenalty ??= this._agentOptions.ChatOptions.FrequencyPenalty;
+        requestChatOptions.Instructions ??= this._agentOptions.ChatOptions.Instructions;
         requestChatOptions.MaxOutputTokens ??= this._agentOptions.ChatOptions.MaxOutputTokens;
         requestChatOptions.ModelId ??= this._agentOptions.ChatOptions.ModelId;
         requestChatOptions.PresencePenalty ??= this._agentOptions.ChatOptions.PresencePenalty;
@@ -250,7 +256,7 @@ public sealed class ChatClientAgent : AIAgent
         {
             foreach (var propertyKey in this._agentOptions.ChatOptions.AdditionalProperties.Keys)
             {
-                requestChatOptions.AdditionalProperties.TryAdd(propertyKey, this._agentOptions.ChatOptions.AdditionalProperties[propertyKey]);
+                _ = requestChatOptions.AdditionalProperties.TryAdd(propertyKey, this._agentOptions.ChatOptions.AdditionalProperties[propertyKey]);
             }
         }
         else
@@ -326,7 +332,7 @@ public sealed class ChatClientAgent : AIAgent
     /// <param name="runOptions">Optional parameters for agent invocation.</param>
     /// <param name="cancellationToken">The cancellation token.</param>
     /// <returns>A tuple containing the thread, chat options, and thread messages.</returns>
-    private async Task<(AgentThread, ChatOptions?, List<ChatMessage>)> PrepareThreadAndMessagesAsync(
+    private async Task<(AgentThread AgentThread, ChatOptions? ChatOptions, List<ChatMessage> ThreadMessages)> PrepareThreadAndMessagesAsync(
         AgentThread? thread,
         IReadOnlyCollection<ChatMessage> inputMessages,
         AgentRunOptions? runOptions,
@@ -343,21 +349,27 @@ public sealed class ChatClientAgent : AIAgent
             threadMessages.Add(message);
         }
 
-        // Update the messages with agent instructions.
-        this.UpdateThreadMessagesWithAgentInstructions(threadMessages, runOptions);
-
         // Add the input messages to the end of thread messages.
         threadMessages.AddRange(inputMessages);
 
         // If a user provided two different thread ids, via the thread object and options, we should throw
         // since we don't know which one to use.
-        if (!string.IsNullOrWhiteSpace(thread.ConversationId) && !string.IsNullOrWhiteSpace(chatOptions?.ConversationId) && thread.ConversationId != chatOptions.ConversationId)
+        if (!string.IsNullOrWhiteSpace(thread.ConversationId) && !string.IsNullOrWhiteSpace(chatOptions?.ConversationId) && thread.ConversationId != chatOptions!.ConversationId)
         {
             throw new InvalidOperationException(
-                $"The {nameof(chatOptions.ConversationId)} provided via {nameof(Microsoft.Extensions.AI.ChatOptions)} is different to the id of the provided {nameof(AgentThread)}. Only one thread id can be used for a run.");
+                $"""
+                The {nameof(chatOptions.ConversationId)} provided via {nameof(Microsoft.Extensions.AI.ChatOptions)} is different to the id of the provided {nameof(AgentThread)}.
+                Only one id can be used for a run.
+                """);
         }
 
-        // Only clone and update ChatOptions if we have an id on the thread and we don't have the same one already in ChatOptions.
+        if (!string.IsNullOrWhiteSpace(this.Instructions))
+        {
+            chatOptions ??= new();
+            chatOptions.Instructions = string.IsNullOrWhiteSpace(chatOptions.Instructions) ? this.Instructions : $"{this.Instructions}\n{chatOptions.Instructions}";
+        }
+
+        // Only create or update ChatOptions if we have an id on the thread and we don't have the same one already in ChatOptions.
         if (!string.IsNullOrWhiteSpace(thread.ConversationId) && thread.ConversationId != chatOptions?.ConversationId)
         {
             chatOptions ??= new();
@@ -373,7 +385,9 @@ public sealed class ChatClientAgent : AIAgent
         {
             // We were passed a thread that is service managed, but we got no conversation id back from the chat client,
             // meaning the service doesn't support service managed threads, so the thread cannot be used with this service.
+#pragma warning disable S2302 // "nameof" should be used - False positive.
             throw new InvalidOperationException("Service did not return a valid conversation id when using a service managed thread.");
+#pragma warning restore S2302 // "nameof" should be used
         }
 
         if (!string.IsNullOrWhiteSpace(responseConversationId))
@@ -391,14 +405,6 @@ public sealed class ChatClientAgent : AIAgent
         }
     }
 
-    private void UpdateThreadMessagesWithAgentInstructions(List<ChatMessage> threadMessages, AgentRunOptions? options)
-    {
-        if (!string.IsNullOrWhiteSpace(this.Instructions))
-        {
-            threadMessages.Insert(0, new(ChatRole.System, this.Instructions) { AuthorName = this.Name });
-        }
-    }
-
     private string GetLoggingAgentName() => this.Name ?? "UnnamedAgent";
     #endregion
 }

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/ChatClientAgentOptions.cs

@@ -79,6 +79,20 @@ public class ChatClientAgentOptions
     /// </summary>
     public Func<IChatMessageStore>? ChatMessageStoreFactory { get; set; } = null;
 
+    /// <summary>
+    /// Gets or sets a value indicating whether to use the provided <see cref="IChatClient"/> instance as is,
+    /// without applying any default decorators.
+    /// </summary>
+    /// <remarks>
+    /// By default the <see cref="ChatClientAgent"/> applies decorators to the provided <see cref="IChatClient"/>
+    /// for doing for example automatic function invocation. Setting this property to <see langword="true"/>
+    /// disables adding these default decorators.
+    /// Disabling is recommended if you want to decorate the <see cref="IChatClient"/> with different decorators
+    /// than the default ones. The provided <see cref="IChatClient"/> instance should then already be decorated
+    /// with the desired decorators.
+    /// </remarks>
+    public bool UseProvidedChatClientAsIs { get; set; } = false;
+
     /// <summary>
     /// Creates a new instance of <see cref="ChatClientAgentOptions"/> with the same values as this instance.
     /// </summary>

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/ChatClientAgentRunOptions.cs

@@ -11,8 +11,8 @@ public sealed class ChatClientAgentRunOptions : AgentRunOptions
     /// Initializes a new instance of the <see cref="ChatClientAgentRunOptions"/> class.
     /// </summary>
     /// <param name="chatOptions">Optional chat options to pass to the agent's invocation.</param>
-    public ChatClientAgentRunOptions(ChatOptions? chatOptions = null) :
-        this(null, chatOptions)
+    public ChatClientAgentRunOptions(ChatOptions? chatOptions = null)
+        : this(null, chatOptions)
     {
     }
 
@@ -26,8 +26,6 @@ public sealed class ChatClientAgentRunOptions : AgentRunOptions
         this.ChatOptions = chatOptions;
     }
 
-    /// <summary>
-    /// Gets or sets optional chat options to pass to the agent's invocation
-    /// </summary>
+    /// <summary>Gets or sets optional chat options to pass to the agent's invocation.</summary>
     public ChatOptions? ChatOptions { get; set; }
 }

dotnet/src/Microsoft.Extensions.AI.Agents/ChatCompletion/ChatClientExtensions.cs

@@ -8,18 +8,18 @@ namespace Microsoft.Extensions.AI.Agents;
 
 internal static class ChatClientExtensions
 {
-    internal static IChatClient AsAgentInvokingChatClient(this IChatClient chatClient)
+    internal static IChatClient AsAgentInvokedChatClient(this IChatClient chatClient)
     {
         var chatBuilder = chatClient.AsBuilder();
 
-        if (chatClient is not AgentInvokingChatClient agentInvokingChatClient)
+        if (chatClient is not AgentInvokedChatClient agentInvokedChatClient)
         {
             chatBuilder.UseAgentInvocation();
         }
 
         if (chatClient.GetService<NewFunctionInvokingChatClient>() is null)
         {
-            chatBuilder.Use((IChatClient innerClient, IServiceProvider services) =>
+            _ = chatBuilder.Use((IChatClient innerClient, IServiceProvider services) =>
             {
                 var loggerFactory = services.GetService<ILoggerFactory>();