Conversation
markwallace-microsoft
added
.NET
workflows
github-actions
bot
changed the title
markwallace-microsoft
added
the
documentation
jozkee
changed the base branch from
main
to
copilot/update-openai-and-microsoft-extensions-ai
jozkee
changed the title
jozkee
changed the base branch from
copilot/update-openai-and-microsoft-extensions-ai
to
main
There was a problem hiding this comment.
Pull request overview
Enhances the .NET DevUI approval workflow to correctly handle MCP tool-call approvals end-to-end (wire parsing, streaming events, storage/replay), and prevents replaying orphaned MCP approval requests that would otherwise cause Azure validation errors.
Changes:
- Added MCP approval response wire-format parsing (
function_approval_response) and enriched streaming approval events to support MCP vs local function approvals. - Implemented MCP tool call/result correlation into a single
mcp_callstored item and replayed it back into chat history. - Added integration tests covering MCP approval flows, orphaned approvals, and local function (FCC) approval behavior in both streaming and non-streaming modes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/TestHelpers.cs | Extends mock chat client to return per-call dynamic content and stream arbitrary content sequences. |
| dotnet/tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/OpenAIResponsesIntegrationTests.cs | Adds integration tests for MCP approvals, orphan handling, and FCC approvals; adds JSON-input helper. |
| dotnet/tests/Microsoft.Agents.AI.Hosting.OpenAI.UnitTests/FunctionApprovalTests.cs | Renames/updates tests to “tool approvals” and adds MCP approval request SSE coverage. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Streaming/McpCallEventGenerator.cs | New generator combining MCP call+result into a single mcp_call output item. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Streaming/FunctionApprovalResponseEventGenerator.cs | Updates approval response streaming to store MCP approval responses as output items and emit DevUI event. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Streaming/FunctionApprovalRequestEventGenerator.cs | Updates approval request streaming to support MCP vs FCC and store MCP approval requests as output items. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Models/StreamingResponseEvent.cs | Extends FunctionCallInfo with optional MCP server_label. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Models/ItemResource.cs | Adds function_approval_response item content type; tightens MCP call item nullability to match spec. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Models/ItemParam.cs | Tightens MCP call param nullability to match spec. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/InMemoryResponsesService.cs | Passes incoming approval response IDs into history replay to avoid misclassifying pending approvals as orphaned. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Converters/ItemResourceConversions.cs | Replays stored MCP approval requests and mcp_call items; adds orphaned-approval skipping logic. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/Converters/ItemContentConverter.cs | Converts DevUI function_approval_response payloads into ToolApprovalResponseContent. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/Responses/AgentResponseUpdateExtensions.cs | Stashes MCP tool calls to correlate with results and uses new generators. |
| dotnet/src/Microsoft.Agents.AI.Hosting.OpenAI/OpenAIHostingJsonUtilities.cs | Registers source-gen serialization type for ItemContentFunctionApprovalResponse. |
| var errorContent = mcpResult.Outputs?.OfType<ErrorContent>().FirstOrDefault(); | ||
| var output = errorContent is null | ||
| ? string.Concat(mcpResult.Outputs?.OfType<TextContent>() ?? []) | ||
| : null; | ||
|
|
||
| var item = new MCPCallItemResource | ||
| { | ||
| Id = idGenerator.GenerateFunctionCallId(), | ||
| ServerLabel = associatedCall.ServerName ?? string.Empty, | ||
| Name = associatedCall.Name, | ||
| Arguments = associatedCall.Arguments is not null | ||
| ? JsonSerializer.Serialize( | ||
| associatedCall.Arguments, | ||
| jsonSerializerOptions.GetTypeInfo(typeof(IDictionary<string, object>))) | ||
| : string.Empty, |
There was a problem hiding this comment.
string.Concat(mcpResult.Outputs?.OfType<TextContent>() ...) will concatenate TextContent objects via ToString() rather than their .Text, producing incorrect Output. Also, ServerLabel and Arguments are now required on MCPCallItemResource, but this code falls back to string.Empty, which is likely invalid (e.g., arguments should be a JSON string like "{}"). Prefer building output from TextContent.Text values, and ensure ServerLabel/Arguments are populated with valid values (e.g., throw if missing server label/name, and default arguments to "{}" when absent).
| var errorContent = mcpResult.Outputs?.OfType<ErrorContent>().FirstOrDefault(); | |
| var output = errorContent is null | |
| ? string.Concat(mcpResult.Outputs?.OfType<TextContent>() ?? []) | |
| : null; | |
| var item = new MCPCallItemResource | |
| { | |
| Id = idGenerator.GenerateFunctionCallId(), | |
| ServerLabel = associatedCall.ServerName ?? string.Empty, | |
| Name = associatedCall.Name, | |
| Arguments = associatedCall.Arguments is not null | |
| ? JsonSerializer.Serialize( | |
| associatedCall.Arguments, | |
| jsonSerializerOptions.GetTypeInfo(typeof(IDictionary<string, object>))) | |
| : string.Empty, | |
| if (string.IsNullOrWhiteSpace(associatedCall.ServerName)) | |
| { | |
| throw new InvalidOperationException($"MCP call for CallId '{mcpResult.CallId}' is missing a required server label."); | |
| } | |
| if (string.IsNullOrWhiteSpace(associatedCall.Name)) | |
| { | |
| throw new InvalidOperationException($"MCP call for CallId '{mcpResult.CallId}' is missing a required tool name."); | |
| } | |
| var errorContent = mcpResult.Outputs?.OfType<ErrorContent>().FirstOrDefault(); | |
| string? output = null; | |
| if (errorContent is null) | |
| { | |
| var textContents = mcpResult.Outputs?.OfType<TextContent>(); | |
| if (textContents is not null) | |
| { | |
| output = string.Concat(textContents.Select(t => t.Text)); | |
| } | |
| } | |
| var item = new MCPCallItemResource | |
| { | |
| Id = idGenerator.GenerateFunctionCallId(), | |
| ServerLabel = associatedCall.ServerName, | |
| Name = associatedCall.Name, | |
| Arguments = associatedCall.Arguments is not null | |
| ? JsonSerializer.Serialize( | |
| associatedCall.Arguments, | |
| jsonSerializerOptions.GetTypeInfo(typeof(IDictionary<string, object>))) | |
| : "{}", |
| @@ -25,14 +25,51 @@ public override IEnumerable<StreamingResponseEvent> ProcessContent(AIContent con | |||
| throw new InvalidOperationException("ToolApprovalResponseEventGenerator only supports ToolApprovalResponseContent."); | |||
There was a problem hiding this comment.
The exception message references ToolApprovalResponseEventGenerator, but the class is now FunctionApprovalResponseEventGenerator. Update the message to match the current type so failures are diagnosable (especially in logs/telemetry).
| throw new InvalidOperationException("ToolApprovalResponseEventGenerator only supports ToolApprovalResponseContent."); | |
| throw new InvalidOperationException("FunctionApprovalResponseEventGenerator only supports ToolApprovalResponseContent."); |
| @@ -27,25 +27,80 @@ public override IEnumerable<StreamingResponseEvent> ProcessContent(AIContent con | |||
| throw new InvalidOperationException("ToolApprovalRequestEventGenerator only supports ToolApprovalRequestContent."); | |||
There was a problem hiding this comment.
The exception message references ToolApprovalRequestEventGenerator, but the class is now FunctionApprovalRequestEventGenerator. Aligning the message with the actual class name will make debugging much clearer.
| throw new InvalidOperationException("ToolApprovalRequestEventGenerator only supports ToolApprovalRequestContent."); | |
| throw new InvalidOperationException("FunctionApprovalRequestEventGenerator only supports ToolApprovalRequestContent."); |
| private readonly Func<int, IEnumerable<ChatMessage>, IList<AIContent>> _contentProvider; | ||
| private int _callIndex; |
There was a problem hiding this comment.
_callIndex++ is not thread-safe. If the same ConversationMemoryMockChatClient instance is ever used concurrently (e.g., parallel tests or overlapping streaming + non-streaming calls), the call index can be duplicated or skipped, causing flaky tests. Consider using Interlocked.Increment(ref _callIndex) - 1 (or similar) to generate a stable per-call index.
| ChatMessage message = new(ChatRole.Assistant, this._responseText); | ||
| var messageList = messages.ToList(); | ||
| this.CallHistory.Add(messageList); | ||
| var contents = this._contentProvider(this._callIndex++, messageList); |
There was a problem hiding this comment.
_callIndex++ is not thread-safe. If the same ConversationMemoryMockChatClient instance is ever used concurrently (e.g., parallel tests or overlapping streaming + non-streaming calls), the call index can be duplicated or skipped, causing flaky tests. Consider using Interlocked.Increment(ref _callIndex) - 1 (or similar) to generate a stable per-call index.
| var messageList = messages.ToList(); | ||
| this.CallHistory.Add(messageList); | ||
| await Task.Delay(1, cancellationToken); | ||
| var contents = this._contentProvider(this._callIndex++, messageList); |
There was a problem hiding this comment.
_callIndex++ is not thread-safe. If the same ConversationMemoryMockChatClient instance is ever used concurrently (e.g., parallel tests or overlapping streaming + non-streaming calls), the call index can be duplicated or skipped, causing flaky tests. Consider using Interlocked.Increment(ref _callIndex) - 1 (or similar) to generate a stable per-call index.
| string callId = string.Empty; | ||
| string name = string.Empty; | ||
| string? serverName = null; | ||
| Dictionary<string, object?>? arguments = null; | ||
|
|
||
| if (approval.FunctionCall is JsonElement fc && fc.ValueKind == JsonValueKind.Object) | ||
| { | ||
| if (fc.TryGetProperty("id", out var idProp)) | ||
| { | ||
| callId = idProp.GetString() ?? string.Empty; | ||
| } | ||
|
|
||
| if (fc.TryGetProperty("name", out var nameProp)) | ||
| { | ||
| name = nameProp.GetString() ?? string.Empty; | ||
| } | ||
|
|
||
| if (fc.TryGetProperty("server_label", out var serverProp)) | ||
| { | ||
| serverName = serverProp.GetString(); | ||
| } | ||
|
|
||
| if (fc.TryGetProperty("arguments", out var argsProp) && argsProp.ValueKind == JsonValueKind.Object) | ||
| { | ||
| arguments = ItemResourceConversions.ParseArguments(argsProp.GetRawText()); | ||
| } | ||
| } | ||
|
|
||
| ToolCallContent toolCall = serverName is not null | ||
| ? new McpServerToolCallContent(callId, name, serverName) { Arguments = arguments } | ||
| : new FunctionCallContent(callId, name, arguments); |
There was a problem hiding this comment.
When function_call is missing or lacks id/name, this constructs McpServerToolCallContent/FunctionCallContent with empty strings. That can silently create malformed tool calls and lead to confusing downstream failures (e.g., mismatch/correlation failures). It would be safer to enforce required fields: if function_call is absent or id/name are null/empty, throw a JsonException (or otherwise reject/ignore the content item) rather than generating an invalid ToolCallContent.
| string callId = string.Empty; | |
| string name = string.Empty; | |
| string? serverName = null; | |
| Dictionary<string, object?>? arguments = null; | |
| if (approval.FunctionCall is JsonElement fc && fc.ValueKind == JsonValueKind.Object) | |
| { | |
| if (fc.TryGetProperty("id", out var idProp)) | |
| { | |
| callId = idProp.GetString() ?? string.Empty; | |
| } | |
| if (fc.TryGetProperty("name", out var nameProp)) | |
| { | |
| name = nameProp.GetString() ?? string.Empty; | |
| } | |
| if (fc.TryGetProperty("server_label", out var serverProp)) | |
| { | |
| serverName = serverProp.GetString(); | |
| } | |
| if (fc.TryGetProperty("arguments", out var argsProp) && argsProp.ValueKind == JsonValueKind.Object) | |
| { | |
| arguments = ItemResourceConversions.ParseArguments(argsProp.GetRawText()); | |
| } | |
| } | |
| ToolCallContent toolCall = serverName is not null | |
| ? new McpServerToolCallContent(callId, name, serverName) { Arguments = arguments } | |
| : new FunctionCallContent(callId, name, arguments); | |
| string? callId = null; | |
| string? name = null; | |
| string? serverName = null; | |
| Dictionary<string, object?>? arguments = null; | |
| if (approval.FunctionCall is not JsonElement fc || fc.ValueKind != JsonValueKind.Object) | |
| { | |
| throw new JsonException("Tool function_call must be a JSON object."); | |
| } | |
| if (fc.TryGetProperty("id", out var idProp)) | |
| { | |
| callId = idProp.GetString(); | |
| } | |
| if (fc.TryGetProperty("name", out var nameProp)) | |
| { | |
| name = nameProp.GetString(); | |
| } | |
| if (fc.TryGetProperty("server_label", out var serverProp)) | |
| { | |
| serverName = serverProp.GetString(); | |
| } | |
| if (fc.TryGetProperty("arguments", out var argsProp) && argsProp.ValueKind == JsonValueKind.Object) | |
| { | |
| arguments = ItemResourceConversions.ParseArguments(argsProp.GetRawText()); | |
| } | |
| if (string.IsNullOrWhiteSpace(callId)) | |
| { | |
| throw new JsonException("Tool function_call is missing required 'id' property."); | |
| } | |
| if (string.IsNullOrWhiteSpace(name)) | |
| { | |
| throw new JsonException("Tool function_call is missing required 'name' property."); | |
| } | |
| ToolCallContent toolCall = serverName is not null | |
| ? new McpServerToolCallContent(callId!, name!, serverName) { Arguments = arguments } | |
| : new FunctionCallContent(callId!, name!, arguments); |
Targets #4613, if that's merged first, that's OK and this must beretargeted to main.Commit 1: Fix MCP approvals in DevUI
item type exists for them)
Commit 2: Add MCP tool call storage
Commit 3: Handle orphaned MCP approval requests
Tagging recent committers: @dmytrostruk @victordibia
cc @stephentoub @rogerbarreto @westey-m