.Net: Fix non-streaming function calling text and usage aggregation - #13429
.Net: Fix non-streaming function calling text and usage aggregation#13429Cozmopolit wants to merge 4 commits into
Conversation
Preserve intermediate LLM text content (e.g., 'Let me check that for you...') and aggregate token usage across all iterations in the auto function calling loop. - Add StringBuilder for text aggregation across loop iterations - Accumulate InputTokens/OutputTokens and store as 'AggregatedUsage' metadata - Apply aggregated state to final response (or filter-terminated response) - Add 5 unit tests covering text aggregation, usage aggregation, single iteration, empty content, and filter termination scenarios Fixes microsoft#13420
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes non-streaming auto function invocation so intermediate assistant text and token usage are preserved/aggregated across loop iterations (OpenAI, Gemini, MistralAI).
Changes:
- Aggregate intermediate assistant text across non-streaming tool-call iterations and prepend it to the final returned message.
- Aggregate token usage across all API calls in the auto-invoke loop and surface aggregated usage on the final/early-terminated result.
- Add OpenAI unit tests + test JSON fixtures covering text aggregation, usage aggregation, and early termination.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Connectors/Connectors.OpenAI/Core/ClientCore.ChatCompletion.cs | Aggregates intermediate text + token usage in non-streaming auto-invoke loop and applies it to the final/terminated message. |
| dotnet/src/Connectors/Connectors.Google/Core/Gemini/Clients/GeminiChatCompletionClient.cs | Adds analogous aggregation logic for Gemini non-streaming auto-invoke loop. |
| dotnet/src/Connectors/Connectors.MistralAI/Client/MistralClient.cs | Adds analogous aggregation logic for Mistral non-streaming auto-invoke loop. |
| dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Core/FunctionCallingContentAggregationTests.cs | New unit tests validating content + usage aggregation behavior for OpenAI connector. |
| dotnet/src/Connectors/Connectors.OpenAI.UnitTests/TestData/*.json | New fixtures for multi-iteration tool call + final response scenarios. |
| dotnet/src/Connectors/Connectors.OpenAI.UnitTests/Connectors.OpenAI.UnitTests.csproj | Ensures new JSON fixtures are copied to test output directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (hadMultipleIterations && message.Metadata is not null) | ||
| { | ||
| var updatedMetadata = new Dictionary<string, object?>(message.Metadata) | ||
| { | ||
| ["AggregatedUsage"] = new Dictionary<string, int> | ||
| { | ||
| ["PromptTokens"] = totalPromptTokens, | ||
| ["CompletionTokens"] = totalCompletionTokens, | ||
| ["TotalTokens"] = totalPromptTokens + totalCompletionTokens | ||
| } | ||
| }; |
| if (hadMultipleIterations && message.Metadata is GeminiMetadata existingMetadata) | ||
| { | ||
| // Create a new metadata dictionary with aggregated values. | ||
| var updatedDict = new Dictionary<string, object?>(existingMetadata) | ||
| { | ||
| [nameof(GeminiMetadata.PromptTokenCount)] = totalPromptTokens, | ||
| [nameof(GeminiMetadata.CandidatesTokenCount)] = totalCandidatesTokens, | ||
| [nameof(GeminiMetadata.TotalTokenCount)] = totalPromptTokens + totalCandidatesTokens | ||
| }; |
| updatedMetadata["AggregatedUsage"] = new Dictionary<string, int> | ||
| { | ||
| ["InputTokens"] = totalInputTokens, | ||
| ["OutputTokens"] = totalOutputTokens, | ||
| ["TotalTokens"] = totalInputTokens + totalOutputTokens | ||
| }; |
| new HttpResponseMessage(HttpStatusCode.OK) | ||
| { | ||
| Content = new StreamContent(File.OpenRead("TestData/aggregation_function_call_with_text_response.json")) | ||
| }, | ||
| // Second response: final text only | ||
| new HttpResponseMessage(HttpStatusCode.OK) | ||
| { | ||
| Content = new StreamContent(File.OpenRead("TestData/aggregation_final_response.json")) | ||
| } |
rogerbarreto
left a comment
There was a problem hiding this comment.
Reviewed PR. Verified locally on merged branch. Findings below.
🔴 Blocking — build broken
1. CS1503 on netstandard2.0. Both ClientCore.ChatCompletion.cs:1100 and MistralClient.cs:1182 do:
new Dictionary<string, object?>(message.Metadata)Metadata is IReadOnlyDictionary. netstandard2.0 Dictionary ctor want IDictionary. Fail.
error CS1503: cannot convert from 'IReadOnlyDictionary<string, object?>' to 'IDictionary<string, object?>'
Reproduce: dotnet build dotnet/src/Connectors/Connectors.OpenAI/Connectors.OpenAI.csproj --tl:off. Same for Mistral. Gemini build OK (use GeminiMetadata.FromDictionary). This explain CI red.
Fix: copy via KeyValuePair enum or .ToDictionary(...).
🔴 Blocking — design / correctness
2. AggregatedUsage schema diverge across 3 connectors. PR desc claim uniform. Reality:
- OpenAI:
{ InputTokens, OutputTokens, TotalTokens } - Mistral:
{ PromptTokens, CompletionTokens, TotalTokens }← different keys - Gemini: no
AggregatedUsagekey. Instead overwritePromptTokenCount/CandidatesTokenCount/TotalTokenCountinGeminiMetadatain place.
Pick one. Suggest InputTokens/OutputTokens (match Microsoft.Extensions.AI.UsageDetails + modern OpenAI SDK).
3. Gemini path = silent breaking change. Caller that read GeminiMetadata.PromptTokenCount for last call now get summed tokens. No opt-out, no doc. Add sibling AggregatedUsage field, leave per-call counts alone.
4. OpenAI filter-terminated path mutate wrong message + pollute history.
FunctionCallsProcessor.ProcessFunctionCallsAsync return last function-result message (role = tool), not assistant. PR call ApplyAggregatedState(lastMessage, …) → prepend assistant prose into tool-result message, overwrite its metadata. Two bugs:
- Semantic: assistant text inside tool-result row.
- Aliasing:
lastMessagesame instance still inchatHistory→ mutation pollute user history.
5. Chat-history duplication on normal path.
Intermediate assistant turns stay in chatHistory with original Content. PR also prepend that content onto returned final message. Common pattern chatHistory.Add(result) → text appear twice in history.
6. Returned Content semantic change. Now "…intermediate…\n\n…final…" blob. String compare / parse / log / display all see new value. No flag, no doc.
🟡 High — test gaps
7. Only OpenAI get tests. Mistral key-name divergence (#2) + Gemini overwrite (#3) immediately catchable with parallel tests. Skipped.
8. NonStreaming_FilterTerminatesEarly_… weak. Assert result.Content contain text. Not assert role of returned message, not assert chatHistory un-mutated. Both miss bug #4.
💡 Suggested redesign
9. Reconsider whole approach. MEAI connectors (AzureAIInference, Ollama) already do this via FunctionInvokingChatClient. Cleaner fix: append intermediate assistant messages to chatHistory during loop, return full IReadOnlyList<ChatMessageContent> (API already shaped for this). Preserve roles, no string stitching, no duplication, per-call usage stay on each message. Worth raise before merge.
⚪ Minor
10. PR desc list InputTokens + OutputTokens but Mistral impl use PromptTokens / CompletionTokens.
11. ReadOnlyDictionary wrap only in OpenAI. Other connectors wrap differently. More inconsistency.
12. Style: Mistral helper static, OpenAI + Gemini instance. Nit.
Verification
dotnet build Connectors.OpenAI → FAIL CS1503 (netstandard2.0)
dotnet build Connectors.MistralAI → FAIL CS1503 (netstandard2.0)
dotnet build Connectors.Google → PASS
Format + test skip until build green.
Recommend block merge until 1 fix + 2 / 3 / 4 resolved. Strongly suggest discuss 9 first.
Motivation and Context
Fixes #13420
When using auto function invocation in non-streaming mode (
GetChatMessageContentAsync), intermediate text content generated by the LLM before tool calls is silently discarded. Additionally, token usage is not aggregated across multiple API calls in the auto-invoke loop.Problem: If the LLM responds with "Let me check that for you..." before requesting a tool call, and then provides a final answer after the tool result, only the final answer is returned. The intermediate text is lost.
Scenario: Users relying on non-streaming mode with auto function invocation expect to receive all text the LLM generated, not just the final response.
Description
This PR modifies the non-streaming auto function calling loop in all three affected connectors to:
StringBuilderAffected Connectors:
Connectors.OpenAI/Core/ClientCore.ChatCompletion.csConnectors.Google/Core/Gemini/Clients/GeminiChatCompletionClient.csConnectors.MistralAI/Client/MistralClient.csImplementation approach:
aggregatedContent(StringBuilder) and token counters\n\nseparator) and token countsAggregatedUsagemetadata (only when multiple iterations occurred)Out of Scope:
FunctionInvokingChatClientNew Tests
Added 5 unit tests for the OpenAI connector in
FunctionCallingContentAggregationTests.cs:NonStreaming_IntermediateTextBeforeToolCall_IsAggregatedInFinalResponseAsyncNonStreaming_TokenUsage_IsAggregatedAcrossAllIterationsAsyncAggregatedUsagemetadata contains sum of all tokensNonStreaming_SingleIteration_NoAggregationMetadataAddedAsyncNonStreaming_ToolCallWithoutIntermediateText_OnlyFinalTextReturnedAsyncNonStreaming_FilterTerminatesEarly_AggregatedContentStillAppliedAsyncContribution Checklist