diff --git a/src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs b/src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs index c1ab00028..044526692 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs @@ -6,4 +6,11 @@ public static class MessageTypeName public const string FunctionCall = "function"; public const string Audio = "audio"; public const string Error = "error"; + + /// + /// A message that belongs to the conversation record but not to the conversation as the user + /// sees it -- what an agent said to itself on the way to an answer. Stored like any other + /// message and read back into the model's context; skipped when the dialog is rendered. + /// + public const string Internal = "internal"; } diff --git a/src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs b/src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs index f7c911c35..6b3a14614 100644 --- a/src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs +++ b/src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs @@ -76,6 +76,23 @@ public class RoleDialogModel : ITrackableMessage [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public string? ToolCallId { get; set; } + /// + /// Every tool call this reply asked for, in the order the model produced them, or null when + /// it asked for none. + /// + /// + /// , and beside + /// this are the first entry, so a caller that can only run one call keeps working unchanged; + /// a caller that can run several reads this instead. The one difference is name repair: the + /// single field carries the normalized name it always has, while entries here keep the name + /// the model actually sent. + /// + /// Deliberately not copied by : this describes one model reply, and a + /// message derived from that reply -- a tool result, an assistant answer -- is not it. + /// + /// + public List? ToolCalls { get; set; } + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public Dictionary? Thought { get; set; } diff --git a/src/Infrastructure/BotSharp.Abstraction/Functions/Models/LlmToolCall.cs b/src/Infrastructure/BotSharp.Abstraction/Functions/Models/LlmToolCall.cs new file mode 100644 index 000000000..a5850ae13 --- /dev/null +++ b/src/Infrastructure/BotSharp.Abstraction/Functions/Models/LlmToolCall.cs @@ -0,0 +1,42 @@ +namespace BotSharp.Abstraction.Functions.Models; + +/// +/// One tool call in a model's reply. +/// +/// +/// A reply can carry several: models routinely ask for independent lookups at once, and every +/// provider here used to keep only the first. See for +/// how the whole set is carried and how it relates to the single-call fields beside it. +/// +public class LlmToolCall +{ + /// + /// The provider's id for this call. It is what a tool result has to be sent back under, so + /// results cannot be matched to calls without it. + /// + public string? Id { get; set; } + + /// + /// The name exactly as the model produced it, with no normalization applied -- a remote MCP + /// tool may legitimately have a name that name repair would rewrite. + /// + public string? FunctionName { get; set; } + + /// + /// Raw JSON arguments. The model does not always produce valid JSON, so parse defensively. + /// + public string? FunctionArgs { get; set; } + + public LlmToolCall() + { + } + + public LlmToolCall(string? id, string? functionName, string? functionArgs) + { + Id = id; + FunctionName = functionName; + FunctionArgs = functionArgs; + } + + public override string ToString() => $"{FunctionName}({FunctionArgs}) [{Id}]"; +} diff --git a/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs b/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs index 8eeee7b35..8da28baae 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/BotSharpMCPExtensions.cs @@ -22,6 +22,11 @@ public static IServiceCollection AddBotSharpMCP(this IServiceCollection services if (settings != null && settings.Enabled && !settings.McpServerConfigs.IsNullOrEmpty()) { + // McpClientManager opens every connection over a client from this factory, so that + // connections to one server share a pooled handler instead of each building its own. + // Idempotent, and a host that already called it is unaffected. + services.AddHttpClient(); + services.AddScoped(); services.AddScoped(); } diff --git a/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs b/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs index ede3dd111..66313e683 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Hooks/MCPToolAgentHook.cs @@ -50,7 +50,7 @@ private async Task> GetMcpContent(Agent agent) var mcps = agent.McpTools?.Where(x => !x.Disabled) ?? []; foreach (var item in mcps) { - var mcpClient = await mcpClientManager.GetMcpClientAsync(item.ServerId); + await using var mcpClient = await mcpClientManager.GetMcpClientAsync(item.ServerId); if (mcpClient == null) continue; var tools = await mcpClient.ListToolsAsync(); diff --git a/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs b/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs index 8e21d74ef..e4249f3d3 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs @@ -1,9 +1,38 @@ using BotSharp.Core.MCP.Settings; using ModelContextProtocol.Client; +using System.Net.Http; namespace BotSharp.Core.MCP.Managers; -public class McpClientManager : IDisposable +/// +/// Opens MCP clients. Each call returns a client of its own, which the caller owns and must +/// dispose; what is shared between callers is the HTTP connection underneath it. +/// +/// +/// +/// WHY NOTHING ABOVE THE SOCKET IS SHARED. An MCP client is a session: CreateAsync performs the +/// initialize handshake, the server answers with a session id, and subscriptions and long-running +/// tool tasks (ListTasksAsync, GetTaskResultAsync) live on that session. Handing one session to +/// two callers would show one of them the other's tasks, and no per-request header can undo that +/// because it is server-side state rather than an authorization question. Since +/// lets a host open a connection as the signed-in user, +/// sharing a session would also mean sharing an identity. So sessions are never shared. +/// +/// +/// WHAT IS SHARED. The HttpClient comes from IHttpClientFactory, named per server, so every +/// connection to one server reuses a pooled HttpMessageHandler -- the same TCP and TLS the +/// factory would give any other caller. That layer carries no identity: the credential lives in +/// the transport's headers, and CreateClient hands back a fresh HttpClient each time, so headers +/// set for one caller are never seen by another. This is what makes a per-call session cheap: +/// the handshake runs over an already-warm connection. +/// +/// +/// Building the transport with its own HttpClient, as this did before, gave every MCP connection +/// a private handler and therefore a private socket pool -- the usual way to exhaust sockets and +/// to keep talking to an address DNS has already moved. +/// +/// +public class McpClientManager { private readonly IServiceProvider _services; private readonly ILogger _logger; @@ -16,12 +45,17 @@ public McpClientManager( _logger = logger; } + /// + /// Opens a client for . The caller owns it and must dispose it + /// -- an undisposed client leaves its session open on the server until the server times it out. + /// Answers null rather than throwing when the server is unknown, disabled or unreachable. + /// public async Task GetMcpClientAsync(string serverId) { try { var settings = _services.GetRequiredService(); - var config = settings.McpServerConfigs.Where(x => x.Id == serverId).FirstOrDefault(); + var config = settings.McpServerConfigs?.FirstOrDefault(x => x.Id == serverId); if (config == null || !config.Enabled) { return null; @@ -30,7 +64,7 @@ public McpClientManager( IClientTransport? transport = null; if (config.HttpConfig != null) { - transport = new HttpClientTransport(new HttpClientTransportOptions + transport = CreateHttpTransport(config, new HttpClientTransportOptions { Name = config.Name, Endpoint = new Uri(config.HttpConfig.EndPoint), @@ -40,7 +74,7 @@ public McpClientManager( } else if (config.SseConfig != null) { - transport = new HttpClientTransport(new HttpClientTransportOptions + transport = CreateHttpTransport(config, new HttpClientTransportOptions { Name = config.Name, Endpoint = new Uri(config.SseConfig.EndPoint), @@ -74,13 +108,40 @@ public McpClientManager( } } + /// + /// A transport over an HttpClient from the factory, named for this server so its handler -- + /// and therefore its connection pool -- is reused by every later connection to the same + /// server. The instance itself is fresh per call, which is what keeps one caller's headers + /// out of another's request. + /// + private HttpClientTransport CreateHttpTransport(McpServerConfigModel config, HttpClientTransportOptions options) + { + var factory = _services.GetRequiredService(); + var http = factory.CreateClient(HttpClientName(config.Id)); + + // Timeout is left at the factory default (100s) deliberately: no configured tool is + // expected to run that long. Note this is a cap the SDK's own HttpClient may not have + // had, so it arrived with this change -- a server whose transport keeps a GET open for + // the session (SSE, or streamable HTTP with a standalone listening stream) would be cut + // off at 100s no matter how quick its tools are. The symptom is a tool call failing with + // a canceled request; the fix is Timeout.InfiniteTimeSpan here. + + return new HttpClientTransport(options, http, loggerFactory: null, ownsHttpClient: true); + } + + /// + /// One handler pool per server, so a slow or unhealthy server cannot occupy the connections + /// of the others. + /// + private static string HttpClientName(string serverId) => $"mcp:{serverId}"; + /// /// The headers to open a connection with: the ones from configuration, unless the host has /// registered an that wants to adjust them. /// /// /// No provider is registered by default, and a provider is free to answer with what it was - /// given, so a host without one — or with one that does not recognise this server — gets the + /// given, so a host without one -- or with one that does not recognise this server -- gets the /// configured headers back untouched. /// private Dictionary? ResolveHeaders(string serverId, Dictionary? configured) @@ -88,9 +149,4 @@ public McpClientManager( var provider = _services.GetService(); return provider == null ? configured : provider.GetHeaders(serverId, configured); } - - public void Dispose() - { - - } } diff --git a/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs b/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs index 7dd20daf6..5b7d1c33c 100644 --- a/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs +++ b/src/Infrastructure/BotSharp.Core/MCP/Services/McpService.cs @@ -28,7 +28,7 @@ public async Task> GetServerConfigsAsync() foreach (var config in configs) { - var client = await clientManager.GetMcpClientAsync(config.Id); + await using var client = await clientManager.GetMcpClientAsync(config.Id); if (client == null) continue; var tools = await client.ListToolsAsync(); diff --git a/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs b/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs index 08be74599..06d345ff1 100644 --- a/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs +++ b/src/Infrastructure/BotSharp.Core/Routing/Executor/MCPToolExecutor.cs @@ -27,7 +27,10 @@ public async Task ExecuteAsync(RoleDialogModel message) Dictionary argDict = JsonToDictionary(message.FunctionArgs); var clientManager = _services.GetRequiredService(); - var client = await clientManager.GetMcpClientAsync(_mcpServerId); + + // The client is a session of its own, so this call owns it. Disposing closes the + // session on the server; the connection underneath it stays in the factory's pool. + await using var client = await clientManager.GetMcpClientAsync(_mcpServerId); if (client == null) { diff --git a/src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs b/src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs index ed8c86b17..d6d5cda32 100644 --- a/src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs +++ b/src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs @@ -117,6 +117,13 @@ public async Task> GetDialogs( var dialogs = new List(); foreach (var message in history) { + // Part of the record, not of the conversation: what an agent said to itself between + // tool calls. It stays in storage and in the model's context, and is not rendered. + if (message.MessageType == MessageTypeName.Internal) + { + continue; + } + if (message.Role == AgentRole.User) { var user = await userService.GetUser(message.SenderId); diff --git a/src/Plugins/BotSharp.Plugin.AnthropicAI/Providers/ChatCompletionProvider.cs b/src/Plugins/BotSharp.Plugin.AnthropicAI/Providers/ChatCompletionProvider.cs index fcb4d4b07..1b909a222 100644 --- a/src/Plugins/BotSharp.Plugin.AnthropicAI/Providers/ChatCompletionProvider.cs +++ b/src/Plugins/BotSharp.Plugin.AnthropicAI/Providers/ChatCompletionProvider.cs @@ -52,14 +52,20 @@ public async Task GetChatCompletions(Agent agent, List new LlmToolCall(x.Id, x.Name, x.Arguments?.ToJsonString())) + .ToList(); + var toolCall = calls.FirstOrDefault(); responseMessage = new RoleDialogModel(AgentRole.Function, string.Empty) { CurrentAgentId = agent.Id, MessageId = conversations.LastOrDefault()?.MessageId ?? string.Empty, ToolCallId = toolCall?.Id, - FunctionName = toolCall?.Name, - FunctionArgs = toolCall?.Arguments?.ToJsonString(), + FunctionName = toolCall?.FunctionName, + FunctionArgs = toolCall?.FunctionArgs, + ToolCalls = calls, RenderedInstruction = string.Join("\r\n", renderedInstructions) }; } @@ -126,14 +132,20 @@ public async Task GetChatCompletionsAsync(Agent agent, List new LlmToolCall(x.Id, x.Name, x.Arguments?.ToJsonString())) + .ToList(); + var toolCall = calls.FirstOrDefault(); responseMessage = new RoleDialogModel(AgentRole.Function, string.Empty) { CurrentAgentId = agent.Id, MessageId = conversations.LastOrDefault()?.MessageId ?? string.Empty, ToolCallId = toolCall?.Id, - FunctionName = toolCall?.Name, - FunctionArgs = toolCall?.Arguments?.ToJsonString(), + FunctionName = toolCall?.FunctionName, + FunctionArgs = toolCall?.FunctionArgs, + ToolCalls = calls, RenderedInstruction = string.Join("\r\n", renderedInstructions) }; @@ -239,18 +251,26 @@ public async Task GetChatCompletionsStreamingAsync(Agent agent, { if (delta.StopReason == StopReason.ToolUse) { - var toolCall = choice.ToolCalls.FirstOrDefault(); + var calls = choice.ToolCalls + .Select(x => new LlmToolCall(x.Id, x.Name, + x.Arguments?.ToString()?.IfNullOrEmptyAs("{}") ?? "{}")) + .ToList(); + var toolCall = calls.FirstOrDefault(); responseMessage = new RoleDialogModel(AgentRole.Function, string.Empty) { CurrentAgentId = agent.Id, MessageId = messageId, ToolCallId = toolCall?.Id, - FunctionName = toolCall?.Name, - FunctionArgs = toolCall?.Arguments?.ToString()?.IfNullOrEmptyAs("{}") ?? "{}" + FunctionName = toolCall?.FunctionName, + FunctionArgs = toolCall?.FunctionArgs ?? "{}", + ToolCalls = calls }; #if DEBUG - _logger.LogDebug($"Tool Call (id: {toolCall?.Id}) => {toolCall?.Name}({toolCall?.Arguments})"); + foreach (var call in calls) + { + _logger.LogDebug($"Tool Call (id: {call.Id}) => {call.FunctionName}({call.FunctionArgs})"); + } #endif } else if (delta.StopReason == StopReason.EndTurn) 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 85f804b7a..fa0c3a1bc 100644 --- a/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs +++ b/src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs @@ -35,14 +35,18 @@ private async Task InnerGetChatCompletions(Agent agent, List x.FunctionName))}"); - var toolCall = value.ToolCalls.FirstOrDefault(); + // Every call the model asked for, not only the first. It routinely asks for several + // independent ones at once, and keeping one made it re-ask for the rest next turn. + var calls = ToLlmToolCalls(value.ToolCalls); + var toolCall = calls.FirstOrDefault(); responseMessage = new RoleDialogModel(AgentRole.Function, text) { CurrentAgentId = agent.Id, MessageId = conversations.LastOrDefault()?.MessageId ?? string.Empty, ToolCallId = toolCall?.Id, FunctionName = toolCall?.FunctionName, - FunctionArgs = toolCall?.FunctionArguments?.ToString(), + FunctionArgs = toolCall?.FunctionArgs, + ToolCalls = calls, RenderedInstruction = string.Join("\r\n", renderedInstructions) }; @@ -148,8 +152,9 @@ private async Task InnerGetChatCompletionsAsync(Agent agent, if (reason == ChatFinishReason.FunctionCall || reason == ChatFinishReason.ToolCalls) { - var toolCall = value.ToolCalls?.FirstOrDefault(); - _logger.LogInformation($"[{agent.Name}]: {toolCall?.FunctionName}({toolCall?.FunctionArguments})"); + var calls = ToLlmToolCalls(value.ToolCalls); + var toolCall = calls.FirstOrDefault(); + _logger.LogInformation($"[{agent.Name}]: {toolCall?.FunctionName}({toolCall?.FunctionArgs})"); var funcContextIn = new RoleDialogModel(AgentRole.Function, text) { @@ -157,7 +162,8 @@ private async Task InnerGetChatCompletionsAsync(Agent agent, MessageId = conversations.LastOrDefault()?.MessageId ?? string.Empty, ToolCallId = toolCall?.Id, FunctionName = toolCall?.FunctionName, - FunctionArgs = toolCall?.FunctionArguments?.ToString(), + FunctionArgs = toolCall?.FunctionArgs, + ToolCalls = calls, RenderedInstruction = string.Join("\r\n", renderedInstructions) }; @@ -278,23 +284,24 @@ private async Task InnerGetChatCompletionsStreamingAsync(Agent if (choice.FinishReason == ChatFinishReason.ToolCalls || choice.FinishReason == ChatFinishReason.FunctionCall) { - var meta = toolCalls.FirstOrDefault(x => !string.IsNullOrEmpty(x.FunctionName)); - var functionName = meta?.FunctionName; - var toolCallId = meta?.ToolCallId; - var args = toolCalls.Where(x => x.FunctionArgumentsUpdate != null).Select(x => x.FunctionArgumentsUpdate.ToString()).ToList(); - var functionArguments = string.Join(string.Empty, args); + var calls = ReconstructToolCalls(toolCalls); + var first = calls.FirstOrDefault(); #if DEBUG - _logger.LogDebug($"Tool Call (id: {toolCallId}) => {functionName}({functionArguments})"); + foreach (var call in calls) + { + _logger.LogDebug($"Tool Call (id: {call.Id}) => {call.FunctionName}({call.FunctionArgs})"); + } #endif responseMessage = new RoleDialogModel(AgentRole.Function, string.Empty) { CurrentAgentId = agent.Id, MessageId = messageId, - ToolCallId = toolCallId, - FunctionName = functionName, - FunctionArgs = functionArguments + ToolCallId = first?.Id, + FunctionName = first?.FunctionName, + FunctionArgs = first?.FunctionArgs ?? string.Empty, + ToolCalls = calls }; } else if (choice.FinishReason == ChatFinishReason.Stop) @@ -743,4 +750,56 @@ private void AddChatToolChoice(ChatCompletionOptions options) } } #endregion + + /// + /// Every tool call in a non-streaming reply, in the order the model produced them. + /// + private static List ToLlmToolCalls(IEnumerable? toolCalls) + => (toolCalls ?? []) + .Select(x => new LlmToolCall(x.Id, x.FunctionName, x.FunctionArguments?.ToString())) + .ToList(); + + /// + /// Rebuilds the tool calls a streaming reply asked for. + /// + /// + /// Arguments arrive chunked across updates and have to be accumulated, and the SDK's update + /// carries no index -- only a tool call id, which is present when a call opens. So an update + /// bearing a new id starts a call, and the fragments following it belong to that one. + /// 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) + { + var calls = new List(); + var args = new List(); + + foreach (var update in updates) + { + var opensCall = calls.Count == 0 + || (!string.IsNullOrEmpty(update.ToolCallId) && calls[^1].Id != update.ToolCallId); + + if (opensCall) + { + calls.Add(new LlmToolCall(update.ToolCallId, update.FunctionName, null)); + args.Add(new StringBuilder()); + } + else if (string.IsNullOrEmpty(calls[^1].FunctionName)) + { + calls[^1].FunctionName = update.FunctionName; + } + + if (update.FunctionArgumentsUpdate != null) + { + args[^1].Append(update.FunctionArgumentsUpdate.ToString()); + } + } + + for (var i = 0; i < calls.Count; i++) + { + calls[i].FunctionArgs = args[i].ToString(); + } + + return calls; + } }