From 7ad0201ba49fa85967d970007eebc88069c67d47 Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Sun, 20 Sep 2026 18:04:40 +0800 Subject: [PATCH 1/2] Run every tool call a reply asked for, not only the first RoleDialogModel.ToolCalls carries the whole set of calls a model reply asked for, and the OpenAI and Anthropic providers have been filling it. Nothing read it. InvokeAgent dispatched on the singular FunctionName beside it -- the first entry -- and the calls behind it were dropped: no error, no log, and nothing said to the model. A reply asking for three independent lookups ran one, and the model either asked for the other two again on the next turn or answered without them. The engine now reads the whole set, runs every call in the order the model produced them, appends every result, and asks the model again once for the batch instead of once per call. The recursion had to move. It lived inside InvokeFunction, at the end of the one call it had just run, which is precisely why nothing after the first call could ever run: the turn was already spent. InvokeFunction now executes one call, appends its result and reports whether the turn should continue; the caller makes that decision once, for the whole batch. Its two existing endings -- StopCompletion, and a rendered response template answering in the function's place -- are unchanged in what they append, they just say so instead of deciding by recursing or not. Sequential, in the order the model gave. Running the set is the fix; running it concurrently is a separate change with a much narrower safe boundary, since IFunctionCallback implementations share IConversationStateService and the routing context. A call that ends the turn ends the batch. The calls behind it were asked for without knowing that, and running them into a finished turn produces results nothing reads. They are skipped with a log line rather than silently, which is the failure this commit is about. Blast radius, deliberately not hidden behind a flag: this changes the behaviour of every existing agent. Prompts that were tuned while only the first call ran -- a prompt that asks for a lookup and a write in one breath and relies on the write being dropped, say -- now get what they asked for. A reply carrying more than one call logs the agent and the tool names at Information, so an agent that starts behaving differently after this can be found from the logs. The response-template branch still does not persist a Function record while the ordinary branch does (223a83c99). That asymmetry is left exactly as it was; each call keeps its own behaviour, and fixing it belongs in its own change. Co-Authored-By: Claude Opus 5 --- .../Routing/RoutingService.InvokeAgent.cs | 188 ++++++--- .../Routing/InvokeAgentToolCallTests.cs | 361 ++++++++++++++++++ 2 files changed, 503 insertions(+), 46 deletions(-) create mode 100644 tests/BotSharp.Core.UnitTests/Routing/InvokeAgentToolCallTests.cs diff --git a/src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs b/src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs index 39be4646d..0b66f3272 100644 --- a/src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs +++ b/src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs @@ -32,7 +32,7 @@ public async Task InvokeAgent( model = agentSettings.LlmConfig.Model; } - var chatCompletion = CompletionProvider.GetChatCompletion(_services, + var chatCompletion = CompletionProvider.GetChatCompletion(_services, provider: provider, model: model); @@ -48,21 +48,10 @@ public async Task InvokeAgent( response = await chatCompletion.GetChatCompletions(agent, conversationDialogs); } - if (response.Role == AgentRole.Function && !string.IsNullOrEmpty(response.FunctionName)) + var toolCalls = GetToolCalls(response); + if (!toolCalls.IsNullOrEmpty()) { - message = RoleDialogModel.From(message, role: AgentRole.Function); - response.FunctionName = response.FunctionName.NormalizeFunctionName(); - message.ToolCallId = response.ToolCallId; - message.FunctionName = response.FunctionName; - message.FunctionArgs = response.FunctionArgs; - message.Thought = response.Thought != null ? new(response.Thought) : null; - message.MetaData = response.MetaData != null ? new(response.MetaData) : null; - message.Indication = response.Indication; - message.CurrentAgentId = agent.Id; - message.IsStreaming = response.IsStreaming; - message.MessageLabel = response.MessageLabel; - - await InvokeFunction(message, dialogs, options); + await InvokeToolCalls(toolCalls, response, message, agent, dialogs, options); } else { @@ -86,6 +75,118 @@ public async Task InvokeAgent( return true; } + /// + /// Every tool call one reply asked for, in the order the model produced them. + /// + /// + /// is where a provider reports the whole set. The + /// single FunctionName/FunctionArgs/ToolCallId fields beside it are a view of its first entry, + /// kept for callers that can only run one call, so a provider that fills only those is read + /// through the second branch and needs no change to work here. + /// + private static List GetToolCalls(RoleDialogModel response) + { + if (!response.ToolCalls.IsNullOrEmpty()) + { + // A call with no name is not something that can be dispatched. The single-call path + // has always guarded on the name being present, and the batch drops it the same way + // rather than handing an empty name to the executor factory. + return response.ToolCalls! + .Where(x => !string.IsNullOrEmpty(x.FunctionName)) + .ToList(); + } + + if (response.Role == AgentRole.Function && !string.IsNullOrEmpty(response.FunctionName)) + { + return [new LlmToolCall(response.ToolCallId, response.FunctionName, response.FunctionArgs)]; + } + + return []; + } + + /// + /// Runs every call the reply asked for, in order, and then hands the whole set of results back + /// to the model in one round. + /// + /// + /// The recursion into belongs here rather than inside + /// : a per-call recursion spends the turn on the first call, and + /// the calls after it never run at all. Deciding once for the whole batch is what lets a model + /// ask for three independent lookups and get three answers in a single round trip. + /// + private async Task InvokeToolCalls( + List toolCalls, + RoleDialogModel response, + RoleDialogModel source, + Agent agent, + List dialogs, + InvokeAgentOptions? options) + { + if (toolCalls.Count > 1) + { + // Worth a line of its own. Until now a reply like this ran one call and dropped the + // rest, so an agent whose prompt was tuned against that behaviour changes the moment + // this ships. This is where to look when one does. + _logger.LogInformation($"Agent {agent.Name} asked for {toolCalls.Count} tool calls in one reply: " + + $"{string.Join(", ", toolCalls.Select(x => x.FunctionName))}"); + } + + var completed = true; + + for (var i = 0; i < toolCalls.Count; i++) + { + var toolCall = toolCalls[i]; + + // Each call in the batch answers the same message. They are siblings of one reply, not + // a chain, so every one is built from the dialog that reply responded to rather than + // from the result the previous call just appended. + var message = RoleDialogModel.From(source, role: AgentRole.Function); + message.ToolCallId = toolCall.Id; + message.FunctionName = toolCall.FunctionName.NormalizeFunctionName(); + message.FunctionArgs = toolCall.FunctionArgs; + message.Thought = response.Thought != null ? new(response.Thought) : null; + message.MetaData = response.MetaData != null ? new(response.MetaData) : null; + message.Indication = response.Indication; + message.CurrentAgentId = agent.Id; + message.IsStreaming = response.IsStreaming; + message.MessageLabel = response.MessageLabel; + + completed = await InvokeFunction(message, dialogs, options); + if (!completed) + { + var skipped = toolCalls.Count - i - 1; + if (skipped > 0) + { + // The model asked for these without knowing an earlier call would end the + // turn. Running them into a finished turn produces results nothing reads, so + // they are dropped -- but not silently, which is how everything beyond the + // first call used to disappear. + _logger.LogInformation($"{message.FunctionName} ended the turn, skipping the remaining {skipped} tool call(s) of this reply: " + + $"{string.Join(", ", toolCalls.Skip(i + 1).Select(x => x.FunctionName))}"); + } + break; + } + } + + if (completed) + { + // One round trip for the whole batch: the model sees every result at once. The agent + // is read after the last call because a routing tool may have changed it. + var routing = _services.GetRequiredService(); + var curAgentId = routing.Context.GetCurrentAgentId(); + await InvokeAgent(curAgentId, dialogs, options); + } + } + + /// + /// Executes one tool call and appends its result to the dialogs. + /// + /// + /// Whether the turn should continue. False when this call ended it -- either the function set + /// and answered the user itself, or a response + /// template answered in its place. Both have always ended the turn; what is new is that the + /// caller is the one told about it, instead of this method deciding by recursing or not. + /// private async Task InvokeFunction( RoleDialogModel message, List dialogs, @@ -101,37 +202,7 @@ private async Task InvokeFunction( var funcOptions = options != null ? new InvokeFunctionOptions() { From = options.From } : null; await routing.InvokeFunction(message.FunctionName, message, options: funcOptions); - // Pass execution result to LLM to get response - if (!message.StopCompletion) - { - // Find response template - var templateService = _services.GetRequiredService(); - var responseTemplate = await templateService.RenderFunctionResponse(message.CurrentAgentId, message); - if (!string.IsNullOrEmpty(responseTemplate)) - { - var msg = RoleDialogModel.From(message, - role: AgentRole.Assistant, - content: responseTemplate); - dialogs.Add(msg); - Context.AddDialogs([msg]); - } - else - { - // Save to memory dialogs and to storage - var msg = RoleDialogModel.From(message, - role: AgentRole.Function, - content: message.Content); - - dialogs.Add(msg); - Context.AddDialogs([msg]); - await Persist(msg); - - // Send to Next LLM - var curAgentId = routing.Context.GetCurrentAgentId(); - await InvokeAgent(curAgentId, dialogs, options); - } - } - else + if (message.StopCompletion) { // The function wrote the reply itself. The call is still worth a record, but kept out // of context, since the assistant message that follows carries the same text. @@ -144,8 +215,33 @@ private async Task InvokeFunction( content: message.Content); dialogs.Add(msg); Context.AddDialogs([msg]); + + return false; } + // Find response template + var templateService = _services.GetRequiredService(); + var responseTemplate = await templateService.RenderFunctionResponse(message.CurrentAgentId, message); + if (!string.IsNullOrEmpty(responseTemplate)) + { + var msg = RoleDialogModel.From(message, + role: AgentRole.Assistant, + content: responseTemplate); + dialogs.Add(msg); + Context.AddDialogs([msg]); + + return false; + } + + // Save to memory dialogs and to storage + var functionMsg = RoleDialogModel.From(message, + role: AgentRole.Function, + content: message.Content); + + dialogs.Add(functionMsg); + Context.AddDialogs([functionMsg]); + await Persist(functionMsg); + return true; } diff --git a/tests/BotSharp.Core.UnitTests/Routing/InvokeAgentToolCallTests.cs b/tests/BotSharp.Core.UnitTests/Routing/InvokeAgentToolCallTests.cs new file mode 100644 index 000000000..627f8883f --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/Routing/InvokeAgentToolCallTests.cs @@ -0,0 +1,361 @@ +using BotSharp.Abstraction.Agents; +using BotSharp.Abstraction.Agents.Enums; +using BotSharp.Abstraction.Agents.Models; +using BotSharp.Abstraction.Agents.Settings; +using BotSharp.Abstraction.Conversations; +using BotSharp.Abstraction.Conversations.Models; +using BotSharp.Abstraction.Functions.Models; +using BotSharp.Abstraction.MLTasks; +using BotSharp.Abstraction.Routing; +using BotSharp.Abstraction.Routing.Models; +using BotSharp.Abstraction.Routing.Settings; +using BotSharp.Abstraction.Templating; +using BotSharp.Core.Routing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace BotSharp.Core.UnitTests.Routing; + +/// +/// A model reply can ask for several tools at once, and every provider here reports the whole set +/// on . The routing engine read only the singular +/// FunctionName beside it, so calls two and three were dropped with no error, no log and nothing +/// told to the model -- it simply asked for them again on the next turn, or answered without them. +/// +/// These tests pin the fixed behaviour: every call runs, results are appended in order, and the +/// model is asked again once for the whole batch rather than once per call. +/// +public class InvokeAgentToolCallTests +{ + private const string AgentId = "agent-1"; + + /// + /// Returns the replies it was given, one per round, and a plain answer once they run out -- + /// which is what stops the recursion at the end of a test. + /// + private sealed class ScriptedChatCompletion(params RoleDialogModel[] replies) : IChatCompletion + { + private readonly Queue _replies = new(replies); + + public string Provider => "test-provider"; + public string Model => "test-model"; + + /// How many times the model was asked. One per LLM round trip. + public int Rounds { get; private set; } + + public void SetModelName(string model) { } + + public Task GetChatCompletions(Agent agent, List conversations) + { + Rounds++; + var reply = _replies.Count > 0 + ? _replies.Dequeue() + : new RoleDialogModel(AgentRole.Assistant, "final answer"); + return Task.FromResult(reply); + } + } + + private sealed record ExecutedCall(string Name, string? ToolCallId, string? Args); + + /// + /// Everything InvokeAgent reaches for, wired to stubs, plus the two things a test looks at: + /// the calls that actually reached the executor and how many LLM rounds it took. + /// + private sealed class Harness + { + public required RoutingService Routing { get; init; } + public required ScriptedChatCompletion Chat { get; init; } + public required List Executed { get; init; } + public required Mock Context { get; init; } + public required Mock Template { get; init; } + } + + private static Harness BuildHarness( + RoleDialogModel[] replies, + Action? onFunctionInvoked = null) + { + var agent = new Agent + { + Id = AgentId, + Name = "tester", + LlmConfig = new AgentLlmConfig + { + Provider = "test-provider", + Model = "test-model", + MaxRecursionDepth = 10 + } + }; + + var agentService = new Mock(); + agentService.Setup(x => x.LoadAgent(It.IsAny(), It.IsAny())).ReturnsAsync(agent); + + var context = new Mock(); + context.Setup(x => x.GetCurrentAgentId()).Returns(AgentId); + + var executed = new List(); + var routingStub = new Mock(); + routingStub.SetupGet(x => x.Context).Returns(context.Object); + routingStub + .Setup(x => x.InvokeFunction(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns((string name, RoleDialogModel message, InvokeFunctionOptions? _) => + { + executed.Add(new ExecutedCall(name, message.ToolCallId, message.FunctionArgs)); + message.Content = $"{name} result"; + onFunctionInvoked?.Invoke(message); + return Task.FromResult(true); + }); + + // No template by default: the branch that renders one ends the turn, and only the test + // that is about that branch wants it. + var template = new Mock(); + template + .Setup(x => x.RenderFunctionResponse(It.IsAny(), It.IsAny())) + .ReturnsAsync(string.Empty); + + // Not in conversation mode, so Persist is a no-op and no storage is needed. + var conversation = new Mock(); + conversation.Setup(x => x.IsConversationMode()).Returns(false); + + var chat = new ScriptedChatCompletion(replies); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(agentService.Object); + services.AddSingleton(new AgentSettings()); + services.AddSingleton(new Mock().Object); + services.AddSingleton(chat); + services.AddSingleton(routingStub.Object); + services.AddSingleton(template.Object); + services.AddSingleton(conversation.Object); + + var routing = new RoutingService( + services.BuildServiceProvider(), + new RoutingSettings(), + context.Object, + NullLogger.Instance); + + return new Harness + { + Routing = routing, + Chat = chat, + Executed = executed, + Context = context, + Template = template + }; + } + + /// A reply asking for the given calls, reported the way every provider reports them. + private static RoleDialogModel ToolReply(params (string Name, string Args, string Id)[] calls) + { + var toolCalls = calls.Select(x => new LlmToolCall(x.Id, x.Name, x.Args)).ToList(); + var first = toolCalls.First(); + + return new RoleDialogModel(AgentRole.Function, string.Empty) + { + CurrentAgentId = AgentId, + ToolCallId = first.Id, + FunctionName = first.FunctionName, + FunctionArgs = first.FunctionArgs, + ToolCalls = toolCalls + }; + } + + private static List NewDialogs() + => [new RoleDialogModel(AgentRole.User, "what is the weather and the time?") { MessageId = "msg-1" }]; + + [Fact] + public async Task InvokeAgent_RunsEveryCallOfTheReply() + { + var harness = BuildHarness( + [ + ToolReply( + ("get_weather", "{\"city\":\"Chicago\"}", "call_1"), + ("get_time", "{\"zone\":\"CST\"}", "call_2"), + ("get_rate", "{\"pair\":\"USDCNY\"}", "call_3")) + ]); + + var dialogs = NewDialogs(); + await harness.Routing.InvokeAgent(AgentId, dialogs); + + Assert.Equal( + ["get_weather", "get_time", "get_rate"], + harness.Executed.Select(x => x.Name)); + + // Each result has to go back under the id of the call it answers, or the provider cannot + // match them up. + Assert.Equal(["call_1", "call_2", "call_3"], harness.Executed.Select(x => x.ToolCallId)); + Assert.Equal( + ["{\"city\":\"Chicago\"}", "{\"zone\":\"CST\"}", "{\"pair\":\"USDCNY\"}"], + harness.Executed.Select(x => x.Args)); + } + + [Fact] + public async Task InvokeAgent_AppendsEveryResultThenAsksTheModelOnce() + { + var harness = BuildHarness( + [ + ToolReply( + ("get_weather", "{}", "call_1"), + ("get_time", "{}", "call_2")) + ]); + + var dialogs = NewDialogs(); + await harness.Routing.InvokeAgent(AgentId, dialogs); + + var functionResults = dialogs.Where(x => x.Role == AgentRole.Function).ToList(); + Assert.Equal(["get_weather result", "get_time result"], functionResults.Select(x => x.Content)); + Assert.Equal(["call_1", "call_2"], functionResults.Select(x => x.ToolCallId)); + + Assert.Equal(AgentRole.Assistant, dialogs.Last().Role); + Assert.Equal("final answer", dialogs.Last().Content); + + // Two rounds, not three: the batch is answered once, not once per call. + Assert.Equal(2, harness.Chat.Rounds); + } + + /// + /// Recursion depth is a budget on how many times the model gets to speak. Running a batch + /// inside one turn must not spend it per call, or a reply asking for four tools would exhaust + /// the default depth of three before the model ever saw a result. + /// + [Fact] + public async Task InvokeAgent_SpendsOneRecursionPerRoundNotPerCall() + { + var harness = BuildHarness( + [ + ToolReply( + ("a", "{}", "call_1"), + ("b", "{}", "call_2"), + ("c", "{}", "call_3")) + ]); + + await harness.Routing.InvokeAgent(AgentId, NewDialogs()); + + harness.Context.Verify(x => x.IncreaseRecursiveCounter(), Times.Exactly(2)); + } + + /// + /// A provider that fills only the singular fields -- every provider outside OpenAI and + /// Anthropic today -- has to keep working untouched. + /// + [Fact] + public async Task InvokeAgent_ReadsTheSingularFieldsWhenToolCallsIsAbsent() + { + var reply = new RoleDialogModel(AgentRole.Function, string.Empty) + { + CurrentAgentId = AgentId, + ToolCallId = "call_1", + FunctionName = "get_weather", + FunctionArgs = "{\"city\":\"Chicago\"}" + }; + + var harness = BuildHarness([reply]); + await harness.Routing.InvokeAgent(AgentId, NewDialogs()); + + var call = Assert.Single(harness.Executed); + Assert.Equal("get_weather", call.Name); + Assert.Equal("call_1", call.ToolCallId); + } + + /// + /// Names arrive on exactly as the model produced them, + /// so the repair that the singular field has always had has to be applied to each of them. + /// + [Fact] + public async Task InvokeAgent_NormalizesEveryCallsName() + { + var harness = BuildHarness( + [ + ToolReply( + ("weather_agent.get_weather", "{}", "call_1"), + ("clock/get_time", "{}", "call_2")) + ]); + + await harness.Routing.InvokeAgent(AgentId, NewDialogs()); + + Assert.Equal(["get_weather", "get_time"], harness.Executed.Select(x => x.Name)); + } + + /// + /// A function that answers the user itself ends the turn. The calls behind it were asked for + /// without knowing that, and running them into a finished turn would produce results nothing + /// reads -- so the batch stops there, and no further round is asked of the model. + /// + [Fact] + public async Task InvokeAgent_StopsAtTheCallThatEndsTheTurn() + { + var harness = BuildHarness( + [ + ToolReply( + ("get_weather", "{}", "call_1"), + ("hand_off_to_human", "{}", "call_2"), + ("get_rate", "{}", "call_3")) + ], + onFunctionInvoked: message => + { + if (message.FunctionName == "hand_off_to_human") + { + message.StopCompletion = true; + message.Content = "A colleague will take it from here."; + } + }); + + var dialogs = NewDialogs(); + await harness.Routing.InvokeAgent(AgentId, dialogs); + + Assert.Equal(["get_weather", "hand_off_to_human"], harness.Executed.Select(x => x.Name)); + Assert.Equal(1, harness.Chat.Rounds); + + Assert.Equal(AgentRole.Assistant, dialogs.Last().Role); + Assert.Equal("A colleague will take it from here.", dialogs.Last().Content); + } + + /// + /// A rendered response template answers in the function's place and has always ended the turn + /// too. It stops the batch for the same reason + /// does. + /// + [Fact] + public async Task InvokeAgent_StopsWhenAResponseTemplateAnswers() + { + var harness = BuildHarness( + [ + ToolReply( + ("get_weather", "{}", "call_1"), + ("get_time", "{}", "call_2")) + ]); + + harness.Template + .Setup(x => x.RenderFunctionResponse(It.IsAny(), It.IsAny())) + .ReturnsAsync("It is sunny in Chicago."); + + var dialogs = NewDialogs(); + await harness.Routing.InvokeAgent(AgentId, dialogs); + + Assert.Equal(["get_weather"], harness.Executed.Select(x => x.Name)); + Assert.Equal(1, harness.Chat.Rounds); + Assert.Equal("It is sunny in Chicago.", dialogs.Last().Content); + } + + /// + /// A reply with no calls at all still ends up as a plain assistant message. + /// + [Fact] + public async Task InvokeAgent_KeepsThePlainAnswerPathUnchanged() + { + var harness = BuildHarness( + [ + new RoleDialogModel(AgentRole.Assistant, "It is sunny.") { CurrentAgentId = AgentId } + ]); + + var dialogs = NewDialogs(); + await harness.Routing.InvokeAgent(AgentId, dialogs); + + Assert.Empty(harness.Executed); + Assert.Equal(1, harness.Chat.Rounds); + Assert.Equal(AgentRole.Assistant, dialogs.Last().Role); + Assert.Equal("It is sunny.", dialogs.Last().Content); + } +} From 5373ad830c06786ebc6cb49b23a0de372404442b Mon Sep 17 00:00:00 2001 From: "mars.yu" Date: Mon, 21 Sep 2026 09:40:04 +0800 Subject: [PATCH 2/2] Cover the streamed reconstruction of a reply's tool calls Reading a reply's tool calls has two implementations. The non-streaming one is handed a finished list and only has to map it. The streaming one rebuilds the list from fragments, and it was the single piece of that path with nothing behind it: no test, and its only logging sits under #if DEBUG, so a Release deployment using it reports nothing at all. Reassembly has to guess because StreamingChatToolCallUpdate exposes only FunctionArgumentsUpdate, FunctionName, Kind and ToolCallId -- no public index. Which call a fragment belongs to is inferred from the tool call id, present when a call opens. Ten tests pin that inference: fragments joined per call, two calls kept apart, two calls of the SAME tool kept apart on id alone, an id repeated on every fragment not reopening a call, a name arriving after its id, a first fragment with no id, a call with no arguments, order, and an empty stream. Checked against the implementation this replaced -- first non-empty name, every argument fragment concatenated into one string -- four of the ten fail. The other six pass, which is the shape of the defect: it is invisible while a reply asks for one tool and produces one malformed argument blob the moment it asks for two. Same tool, twice, is not hypothetical. Production replies carry up to four copies of one tool name in a single reply, and the id is the only thing telling those apart, so grouping must never fall back to the name. ReconstructToolCalls becomes internal, with InternalsVisibleTo for the test assembly, rather than being exercised through a live stream. Co-Authored-By: Claude Opus 5 --- .../BotSharp.Plugin.OpenAI.csproj | 6 + .../Chat/ChatCompletionProvider.Chat.cs | 2 +- .../BotSharp.Core.UnitTests.csproj | 2 + .../StreamingToolCallReconstructionTests.cs | 215 ++++++++++++++++++ 4 files changed, 224 insertions(+), 1 deletion(-) create mode 100644 tests/BotSharp.Core.UnitTests/Routing/StreamingToolCallReconstructionTests.cs diff --git a/src/Plugins/BotSharp.Plugin.OpenAI/BotSharp.Plugin.OpenAI.csproj b/src/Plugins/BotSharp.Plugin.OpenAI/BotSharp.Plugin.OpenAI.csproj index 7fc22b57a..2515ae550 100644 --- a/src/Plugins/BotSharp.Plugin.OpenAI/BotSharp.Plugin.OpenAI.csproj +++ b/src/Plugins/BotSharp.Plugin.OpenAI/BotSharp.Plugin.OpenAI.csproj @@ -23,4 +23,10 @@ + + + + + \ No newline at end of file diff --git a/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs b/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs index a0060e85e..f731a9842 100644 --- a/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs +++ b/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs @@ -769,7 +769,7 @@ private static List ToLlmToolCalls(IEnumerable? toolC /// Concatenating every fragment into a single string, as this did before, produced one /// malformed argument blob as soon as the model asked for more than one tool at a time. /// - private static List ReconstructToolCalls(List updates) + internal static List ReconstructToolCalls(List updates) { var calls = new List(); var args = new List(); diff --git a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj index a56498e7e..46f2f5e5b 100644 --- a/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj +++ b/tests/BotSharp.Core.UnitTests/BotSharp.Core.UnitTests.csproj @@ -34,6 +34,8 @@ + + diff --git a/tests/BotSharp.Core.UnitTests/Routing/StreamingToolCallReconstructionTests.cs b/tests/BotSharp.Core.UnitTests/Routing/StreamingToolCallReconstructionTests.cs new file mode 100644 index 000000000..461d76d67 --- /dev/null +++ b/tests/BotSharp.Core.UnitTests/Routing/StreamingToolCallReconstructionTests.cs @@ -0,0 +1,215 @@ +using BotSharp.Abstraction.Functions.Models; +using BotSharp.Plugin.OpenAI.Providers.Chat; +using OpenAI.Chat; +using Xunit; + +namespace BotSharp.Core.UnitTests.Routing; + +/// +/// The streamed counterpart of reading a reply's tool calls. A non-streaming reply hands over a +/// finished list; a streamed one arrives as fragments that have to be reassembled, and that +/// reassembly is the only part of the path with no production evidence behind it -- the streaming +/// branch logs its calls under #if DEBUG, so a Release deployment that uses it shows +/// nothing at all. +/// +/// +/// Why reassembly is guesswork in the first place: in +/// the OpenAI SDK exposes only FunctionArgumentsUpdate, FunctionName, Kind and ToolCallId. There +/// is no public index, so which call a fragment belongs to has to be inferred -- an update +/// carrying a tool call id different from the one in progress starts a new call, and everything +/// after it belongs to that one. +/// +/// The case these exist for: two calls in one reply. Appending every fragment to a single string, +/// as this did originally, is invisible while a reply asks for one tool and produces a single +/// malformed argument blob the moment it asks for two. +/// +/// +public class StreamingToolCallReconstructionTests +{ + /// + /// One streamed fragment. Null means the update continues the call in + /// progress, which is how the SDK reports every fragment after the first of a call. + /// + private static StreamingChatToolCallUpdate Update(string? id = null, string? name = null, string? args = null) + => OpenAIChatModelFactory.StreamingChatToolCallUpdate( + index: 0, + toolCallId: id!, + kind: ChatToolCallKind.Function, + functionName: name!, + functionArgumentsUpdate: args != null ? BinaryData.FromString(args) : null!); + + [Fact] + public void Reconstruct_JoinsTheFragmentsOfOneCall() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "get_weather", args: "{\"ci"), + Update(args: "ty\":\"Chi"), + Update(args: "cago\"}") + ]); + + var call = Assert.Single(calls); + Assert.Equal("call_1", call.Id); + Assert.Equal("get_weather", call.FunctionName); + Assert.Equal("{\"city\":\"Chicago\"}", call.FunctionArgs); + } + + /// + /// The regression this reassembly exists for. Both calls must come out whole and separate; + /// the failure it replaced produced one call whose arguments were the two JSON documents + /// concatenated. + /// + [Fact] + public void Reconstruct_KeepsTwoCallsArgumentsApart() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "get_weather", args: "{\"city\":"), + Update(args: "\"Chicago\"}"), + Update(id: "call_2", name: "get_time", args: "{\"zone\":"), + Update(args: "\"CST\"}") + ]); + + Assert.Equal(2, calls.Count); + Assert.Equal(["call_1", "call_2"], calls.Select(x => x.Id)); + Assert.Equal(["get_weather", "get_time"], calls.Select(x => x.FunctionName)); + Assert.Equal("{\"city\":\"Chicago\"}", calls[0].FunctionArgs); + Assert.Equal("{\"zone\":\"CST\"}", calls[1].FunctionArgs); + } + + /// + /// A reply really does ask for the same tool more than once -- production shows models doing + /// it with up to four copies of one name in a single reply. The id is the only thing telling + /// those apart, so grouping must not fall back to the name. + /// + [Fact] + public void Reconstruct_SeparatesTwoCallsOfTheSameTool() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "process_wo_avoidance", args: "{\"key_words\":\"toilet leak\"}"), + Update(id: "call_2", name: "process_wo_avoidance", args: "{\"key_words\":\"roof repair\"}") + ]); + + Assert.Equal(2, calls.Count); + Assert.Equal(["call_1", "call_2"], calls.Select(x => x.Id)); + Assert.Equal("{\"key_words\":\"toilet leak\"}", calls[0].FunctionArgs); + Assert.Equal("{\"key_words\":\"roof repair\"}", calls[1].FunctionArgs); + } + + /// + /// A provider that stamps the id on every fragment rather than only on the first must not be + /// read as opening a new call per fragment. + /// + [Fact] + public void Reconstruct_DoesNotReopenACallWhenTheIdRepeats() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "get_weather", args: "{\"city\":"), + Update(id: "call_1", args: "\"Chicago\"}") + ]); + + var call = Assert.Single(calls); + Assert.Equal("{\"city\":\"Chicago\"}", call.FunctionArgs); + } + + /// + /// The name does not have to arrive with the id. When it comes in a later fragment it still + /// has to land on the call in progress, or the call is dispatched under an empty name. + /// + [Fact] + public void Reconstruct_TakesTheNameFromALaterFragment() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1"), + Update(name: "get_weather"), + Update(args: "{}") + ]); + + var call = Assert.Single(calls); + Assert.Equal("call_1", call.Id); + Assert.Equal("get_weather", call.FunctionName); + Assert.Equal("{}", call.FunctionArgs); + } + + /// + /// A first fragment with no id still opens a call rather than indexing into an empty list. + /// + [Fact] + public void Reconstruct_OpensACallEvenWhenTheFirstFragmentHasNoId() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(name: "get_weather", args: "{\"city\":"), + Update(args: "\"Chicago\"}") + ]); + + var call = Assert.Single(calls); + Assert.Equal("get_weather", call.FunctionName); + Assert.Equal("{\"city\":\"Chicago\"}", call.FunctionArgs); + } + + /// + /// Arguments are never null on the way out: a call the model sent no arguments for has to + /// serialize as something the provider can put in the request. + /// + [Fact] + public void Reconstruct_GivesACallWithoutArgumentsAnEmptyString() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "list_work_orders") + ]); + + var call = Assert.Single(calls); + Assert.Equal(string.Empty, call.FunctionArgs); + } + + [Fact] + public void Reconstruct_ReturnsNothingForAnEmptyStream() + { + Assert.Empty(ChatCompletionProvider.ReconstructToolCalls([])); + } + + /// + /// Order is the contract the rest of the engine reads: results are appended in the order the + /// model asked, and a reply that stops the turn part way through keeps the calls before it. + /// + [Fact] + public void Reconstruct_KeepsTheOrderTheModelSentThemIn() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "a", args: "{}"), + Update(id: "call_2", name: "b", args: "{}"), + Update(id: "call_3", name: "c", args: "{}") + ]); + + Assert.Equal(["a", "b", "c"], calls.Select(x => x.FunctionName)); + } + + /// + /// What the engine reads afterwards. is the shape the routing side + /// dispatches on, and a call is only dispatchable with both an id to answer under and a name + /// to resolve. + /// + [Fact] + public void Reconstruct_ProducesCallsTheEngineCanDispatch() + { + var calls = ChatCompletionProvider.ReconstructToolCalls( + [ + Update(id: "call_1", name: "get_weather", args: "{\"city\":\"Chicago\"}"), + Update(id: "call_2", name: "get_time", args: "{\"zone\":\"CST\"}") + ]); + + Assert.Equal(2, calls.Count); + Assert.All(calls, call => + { + Assert.False(string.IsNullOrEmpty(call.Id)); + Assert.False(string.IsNullOrEmpty(call.FunctionName)); + Assert.NotNull(call.FunctionArgs); + }); + } +}