Address PR feedback

Author: stephentoub

src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIAssistantChatClient.cs

@@ -42,21 +42,22 @@ internal sealed partial class OpenAIAssistantChatClient : IChatClient
     private readonly string _assistantId;
 
     /// <summary>The thread ID to use if none is supplied in <see cref="ChatOptions.ConversationId"/>.</summary>
-    private readonly string? _threadId;
+    private readonly string? _defaultThreadId;
 
     /// <summary>Initializes a new instance of the <see cref="OpenAIAssistantChatClient"/> class for the specified <see cref="AssistantClient"/>.</summary>
-    public OpenAIAssistantChatClient(AssistantClient client, string assistantId, string? threadId)
+    public OpenAIAssistantChatClient(AssistantClient assistantClient, string assistantId, string? defaultThreadId)
     {
-        _client = Throw.IfNull(client);
+        _client = Throw.IfNull(assistantClient);
         _assistantId = Throw.IfNullOrWhitespace(assistantId);
-        _threadId = threadId;
+
+        _defaultThreadId = defaultThreadId;
 
         // https://github.com/openai/openai-dotnet/issues/215
         // The endpoint isn't currently exposed, so use reflection to get at it, temporarily. Once packages
         // implement the abstractions directly rather than providing adapters on top of the public APIs,
         // the package can provide such implementations separate from what's exposed in the public API.
         Uri providerUrl = typeof(AssistantClient).GetField("_endpoint", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
-            ?.GetValue(client) as Uri ?? OpenAIResponseChatClient.DefaultOpenAIEndpoint;
+            ?.GetValue(assistantClient) as Uri ?? OpenAIResponseChatClient.DefaultOpenAIEndpoint;
 
         _metadata = new("openai", providerUrl);
     }
@@ -85,7 +86,7 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
         (RunCreationOptions runOptions, List<FunctionResultContent>? toolResults) = CreateRunOptions(messages, options);
 
         // Get the thread ID.
-        string? threadId = options?.ConversationId ?? _threadId;
+        string? threadId = options?.ConversationId ?? _defaultThreadId;
         if (threadId is null && toolResults is not null)
         {
             Throw.ArgumentException(nameof(messages), "No thread ID was provided, but chat messages includes tool results.");
@@ -327,17 +328,22 @@ void IDisposable.Dispose()
             }
         }
 
-        // Process ChatMessages. System messages are turned into additional instructions.
-        // All other messages are added 1:1, treating assistant messages as agent messages
-        // and everything else as user messages.
+        // Process ChatMessages.
         StringBuilder? instructions = null;
         List<FunctionResultContent>? functionResults = null;
         foreach (var chatMessage in messages)
         {
             List<MessageContent> messageContents = [];
 
+            // Assistants doesn't support system/developer messages directly. It does support transient per-request instructions,
+            // so we can use the system/developer messages to build up a set of instructions that will be passed to the assistant
+            // as part of this request. However, in doing so, on a subsequent request that information will be lost, as there's no
+            // way to store per-thread instructions in the OpenAI Assistants API. We don't want to convert these to user messages,
+            // however, as that would then expose the system/developer messages in a way that might make the model more likely
+            // to include that information in its responses. System messages should ideally be instead done as instructions to
+            // the assistant when the assistant is created.
             if (chatMessage.Role == ChatRole.System ||
-                chatMessage.Role == OpenAIChatClient.ChatRoleDeveloper)
+                chatMessage.Role == OpenAIResponseChatClient.ChatRoleDeveloper)
             {
                 instructions ??= new();
                 foreach (var textContent in chatMessage.Contents.OfType<TextContent>())

src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIChatClient.cs

@@ -110,8 +110,6 @@ void IDisposable.Dispose()
         // Nothing to dispose. Implementation required for the IChatClient interface.
     }
 
-    internal static ChatRole ChatRoleDeveloper { get; } = new ChatRole("developer");
-
     /// <summary>Converts an Extensions chat message enumerable to an OpenAI chat message enumerable.</summary>
     private static IEnumerable<OpenAI.Chat.ChatMessage> ToOpenAIChatMessages(IEnumerable<ChatMessage> inputs, JsonSerializerOptions options)
     {
@@ -122,12 +120,12 @@ void IDisposable.Dispose()
         {
             if (input.Role == ChatRole.System ||
                 input.Role == ChatRole.User ||
-                input.Role == ChatRoleDeveloper)
+                input.Role == OpenAIResponseChatClient.ChatRoleDeveloper)
             {
                 var parts = ToOpenAIChatContent(input.Contents);
                 yield return
                     input.Role == ChatRole.System ? new SystemChatMessage(parts) { ParticipantName = input.AuthorName } :
-                    input.Role == ChatRoleDeveloper ? new DeveloperChatMessage(parts) { ParticipantName = input.AuthorName } :
+                    input.Role == OpenAIResponseChatClient.ChatRoleDeveloper ? new DeveloperChatMessage(parts) { ParticipantName = input.AuthorName } :
                     new UserChatMessage(parts) { ParticipantName = input.AuthorName };
             }
             else if (input.Role == ChatRole.Tool)
@@ -619,7 +617,7 @@ private static ChatRole FromOpenAIChatRole(ChatMessageRole role) =>
             ChatMessageRole.User => ChatRole.User,
             ChatMessageRole.Assistant => ChatRole.Assistant,
             ChatMessageRole.Tool => ChatRole.Tool,
-            ChatMessageRole.Developer => ChatRoleDeveloper,
+            ChatMessageRole.Developer => OpenAIResponseChatClient.ChatRoleDeveloper,
             _ => new ChatRole(role.ToString()),
         };
 

src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIClientExtensions.cs

@@ -27,7 +27,7 @@ public static IChatClient AsIChatClient(this OpenAIResponseClient responseClient
         new OpenAIResponseChatClient(responseClient);
 
     /// <summary>Gets an <see cref="IChatClient"/> for use with this <see cref="AssistantClient"/>.</summary>
-    /// <param name="client">The <see cref="AssistantClient"/> instance to be accessed as an <see cref="IChatClient"/>.</param>
+    /// <param name="assistantClient">The <see cref="AssistantClient"/> instance to be accessed as an <see cref="IChatClient"/>.</param>
     /// <param name="assistantId">The unique identifier of the assistant with which to interact.</param>
     /// <param name="threadId">
     /// An optional existing thread identifier for the chat session. This serves as a default, and may be overridden per call to
@@ -36,8 +36,8 @@ public static IChatClient AsIChatClient(this OpenAIResponseClient responseClient
     /// </param>
     /// <returns>An <see cref="IChatClient"/> instance configured to interact with the specified agent and thread.</returns>
     [Experimental("OPENAI001")]
-    public static IChatClient AsIChatClient(this AssistantClient client, string assistantId, string? threadId = null) =>
-        new OpenAIAssistantChatClient(client, assistantId, threadId);
+    public static IChatClient AsIChatClient(this AssistantClient assistantClient, string assistantId, string? threadId = null) =>
+        new OpenAIAssistantChatClient(assistantClient, assistantId, threadId);
 
     /// <summary>Gets an <see cref="ISpeechToTextClient"/> for use with this <see cref="AudioClient"/>.</summary>
     /// <param name="audioClient">The client.</param>

src/Libraries/Microsoft.Extensions.AI.OpenAI/OpenAIResponseChatClient.cs

@@ -29,6 +29,9 @@ internal sealed partial class OpenAIResponseChatClient : IChatClient
     /// <summary>Gets the default OpenAI endpoint.</summary>
     internal static Uri DefaultOpenAIEndpoint { get; } = new("https://api.openai.com/v1");
 
+    /// <summary>Gets a <see cref="ChatRole"/> for "developer".</summary>
+    internal static ChatRole ChatRoleDeveloper { get; } = new ChatRole("developer");
+
     /// <summary>Metadata about the client.</summary>
     private readonly ChatClientMetadata _metadata;
 
@@ -85,7 +88,7 @@ public async Task<ChatResponse> GetResponseAsync(
         // Convert and return the results.
         ChatResponse response = new()
         {
-            ConversationId = openAIResponse.Id,
+            ConversationId = openAIOptions.StoredOutputEnabled is false ? null : openAIResponse.Id,
             CreatedAt = openAIResponse.CreatedAt,
             FinishReason = ToFinishReason(openAIResponse.IncompleteStatusDetails?.Reason),
             Messages = [new(ChatRole.Assistant, [])],
@@ -164,6 +167,7 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
         // Make the call to the OpenAIResponseClient and process the streaming results.
         DateTimeOffset? createdAt = null;
         string? responseId = null;
+        string? conversationId = null;
         string? modelId = null;
         string? lastMessageId = null;
         ChatRole? lastRole = null;
@@ -176,18 +180,19 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                 case StreamingResponseCreatedUpdate createdUpdate:
                     createdAt = createdUpdate.Response.CreatedAt;
                     responseId = createdUpdate.Response.Id;
+                    conversationId = openAIOptions.StoredOutputEnabled is false ? null : responseId;
                     modelId = createdUpdate.Response.Model;
                     goto default;
 
                 case StreamingResponseCompletedUpdate completedUpdate:
                     yield return new()
                     {
+                        Contents = ToUsageDetails(completedUpdate.Response) is { } usage ? [new UsageContent(usage)] : [],
+                        ConversationId = conversationId,
+                        CreatedAt = createdAt,
                         FinishReason =
                             ToFinishReason(completedUpdate.Response?.IncompleteStatusDetails?.Reason) ??
                             (functionCallInfos is not null ? ChatFinishReason.ToolCalls : ChatFinishReason.Stop),
-                        Contents = ToUsageDetails(completedUpdate.Response) is { } usage ? [new UsageContent(usage)] : [],
-                        ConversationId = responseId,
-                        CreatedAt = createdAt,
                         MessageId = lastMessageId,
                         ModelId = modelId,
                         RawRepresentation = streamingUpdate,
@@ -220,7 +225,7 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                     lastRole = ToChatRole(messageItem?.Role);
                     yield return new ChatResponseUpdate(lastRole, outputTextDeltaUpdate.Delta)
                     {
-                        ConversationId = responseId,
+                        ConversationId = conversationId,
                         CreatedAt = createdAt,
                         MessageId = lastMessageId,
                         ModelId = modelId,
@@ -255,7 +260,7 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                         lastRole = ChatRole.Assistant;
                         yield return new ChatResponseUpdate(lastRole, [fci])
                         {
-                            ConversationId = responseId,
+                            ConversationId = conversationId,
                             CreatedAt = createdAt,
                             MessageId = lastMessageId,
                             ModelId = modelId,
@@ -272,7 +277,6 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                 case StreamingResponseErrorUpdate errorUpdate:
                     yield return new ChatResponseUpdate
                     {
-                        ConversationId = responseId,
                         Contents =
                         [
                             new ErrorContent(errorUpdate.Message)
@@ -281,6 +285,7 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                                 Details = errorUpdate.Param,
                             }
                         ],
+                        ConversationId = conversationId,
                         CreatedAt = createdAt,
                         MessageId = lastMessageId,
                         ModelId = modelId,
@@ -293,21 +298,21 @@ public async IAsyncEnumerable<ChatResponseUpdate> GetStreamingResponseAsync(
                 case StreamingResponseRefusalDoneUpdate refusalDone:
                     yield return new ChatResponseUpdate
                     {
+                        Contents = [new ErrorContent(refusalDone.Refusal) { ErrorCode = nameof(ResponseContentPart.Refusal) }],
+                        ConversationId = conversationId,
                         CreatedAt = createdAt,
                         MessageId = lastMessageId,
                         ModelId = modelId,
                         RawRepresentation = streamingUpdate,
                         ResponseId = responseId,
                         Role = lastRole,
-                        ConversationId = responseId,
-                        Contents = [new ErrorContent(refusalDone.Refusal) { ErrorCode = nameof(ResponseContentPart.Refusal) }],
                     };
                     break;
 
                 default:
                     yield return new ChatResponseUpdate
                     {
-                        ConversationId = responseId,
+                        ConversationId = conversationId,
                         CreatedAt = createdAt,
                         MessageId = lastMessageId,
                         ModelId = modelId,
@@ -331,7 +336,7 @@ private static ChatRole ToChatRole(MessageRole? role) =>
         role switch
         {
             MessageRole.System => ChatRole.System,
-            MessageRole.Developer => OpenAIChatClient.ChatRoleDeveloper,
+            MessageRole.Developer => ChatRoleDeveloper,
             MessageRole.User => ChatRole.User,
             _ => ChatRole.Assistant,
         };

test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIAssistantChatClientIntegrationTests.cs

@@ -9,6 +9,7 @@
 
 using System;
 using System.Linq;
+using System.Net;
 using System.Net.Http;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
@@ -45,26 +46,21 @@ public class OpenAIAssistantChatClientIntegrationTests : ChatClientIntegrationTe
     public override Task MultiModal_DescribeImage() => Task.CompletedTask;
     public override Task MultiModal_DescribePdf() => Task.CompletedTask;
 
-    // [Fact]
+    // [Fact] // uncomment and run to clear out _all_ threads in your OpenAI account
     public async Task DeleteAllThreads()
     {
         using HttpClient client = new(new HttpClientHandler
         {
-            AutomaticDecompression = System.Net.DecompressionMethods.GZip,
+            AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate,
         });
 
-        client.DefaultRequestHeaders.Add("accept", "*/*");
-        client.DefaultRequestHeaders.Add("accept-encoding", "gzip");
-        client.DefaultRequestHeaders.Add("accept-language", "en-US,en;q=0.9");
-        client.DefaultRequestHeaders.Add("openai-beta", "assistants=v2");
-        client.DefaultRequestHeaders.Add("origin", "https://platform.openai.com");
-        client.DefaultRequestHeaders.Add("user-agent", "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/137.0.0.0 Safari/537.36 Edg/137.0.0.0");
-
         // These values need to be filled in. The bearer token needs to be sniffed from a browser
-        // session interacting with the dashboard (e.g. F12 networking tools).
-        client.DefaultRequestHeaders.Add("authorization", $"Bearer TODO-SESSION-TOKEN");
-        client.DefaultRequestHeaders.Add("openai-organization", "TODO");
-        client.DefaultRequestHeaders.Add("openai-project", "TODO");
+        // session interacting with the dashboard (e.g. use F12 networking tools to look at request headers
+        // made to "https://api.openai.com/v1/threads?limit=10" after clicking on Assistants | Threads in the
+        // OpenAI portal dashboard).
+        client.DefaultRequestHeaders.Add("authorization", $"Bearer sess-ENTERYOURSESSIONTOKEN");
+        client.DefaultRequestHeaders.Add("openai-organization", "org-ENTERYOURORGID");
+        client.DefaultRequestHeaders.Add("openai-project", "proj_ENTERYOURPROJECTID");
 
         AssistantClient ac = new AssistantClient(Environment.GetEnvironmentVariable("AI:OpenAI:ApiKey")!);
         while (true)

test/Libraries/Microsoft.Extensions.AI.OpenAI.Tests/OpenAIAssistantChatClientTests.cs

@@ -0,0 +1,77 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.ClientModel;
+using Azure.AI.OpenAI;
+using Microsoft.Extensions.Caching.Distributed;
+using Microsoft.Extensions.Caching.Memory;
+using OpenAI;
+using OpenAI.Assistants;
+using Xunit;
+
+#pragma warning disable S103 // Lines should not be too long
+#pragma warning disable OPENAI001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
+
+namespace Microsoft.Extensions.AI;
+
+public class OpenAIAssistantChatClientTests
+{
+    [Fact]
+    public void AsIChatClient_InvalidArgs_Throws()
+    {
+        Assert.Throws<ArgumentNullException>("assistantClient", () => ((AssistantClient)null!).AsIChatClient("assistantId"));
+        Assert.Throws<ArgumentNullException>("assistantId", () => new AssistantClient("ignored").AsIChatClient(null!));
+    }
+
+    [Theory]
+    [InlineData(false)]
+    [InlineData(true)]
+    public void AsIChatClient_OpenAIClient_ProducesExpectedMetadata(bool useAzureOpenAI)
+    {
+        Uri endpoint = new("http://localhost/some/endpoint");
+
+        var client = useAzureOpenAI ?
+            new AzureOpenAIClient(endpoint, new ApiKeyCredential("key")) :
+            new OpenAIClient(new ApiKeyCredential("key"), new OpenAIClientOptions { Endpoint = endpoint });
+
+        IChatClient[] clients =
+        [
+            client.GetAssistantClient().AsIChatClient("assistantId"),
+            client.GetAssistantClient().AsIChatClient("assistantId", "threadId"),
+        ];
+
+        foreach (var chatClient in clients)
+        {
+            var metadata = chatClient.GetService<ChatClientMetadata>();
+            Assert.Equal("openai", metadata?.ProviderName);
+            Assert.Equal(endpoint, metadata?.ProviderUri);
+        }
+    }
+
+    [Fact]
+    public void GetService_AssistantClient_SuccessfullyReturnsUnderlyingClient()
+    {
+        AssistantClient assistantClient = new OpenAIClient("key").GetAssistantClient();
+        IChatClient chatClient = assistantClient.AsIChatClient("assistantId");
+
+        Assert.Same(assistantClient, chatClient.GetService<AssistantClient>());
+
+        Assert.Null(chatClient.GetService<OpenAIClient>());
+
+        using IChatClient pipeline = chatClient
+            .AsBuilder()
+            .UseFunctionInvocation()
+            .UseOpenTelemetry()
+            .UseDistributedCache(new MemoryDistributedCache(Options.Options.Create(new MemoryDistributedCacheOptions())))
+            .Build();
+
+        Assert.NotNull(pipeline.GetService<FunctionInvokingChatClient>());
+        Assert.NotNull(pipeline.GetService<DistributedCachingChatClient>());
+        Assert.NotNull(pipeline.GetService<CachingChatClient>());
+        Assert.NotNull(pipeline.GetService<OpenTelemetryChatClient>());
+
+        Assert.Same(assistantClient, pipeline.GetService<AssistantClient>());
+        Assert.IsType<FunctionInvokingChatClient>(pipeline.GetService<IChatClient>());
+    }
+}