From 20a8ec7c1aaa011102fb5908861af9c518fc90b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 13:51:28 +0000 Subject: [PATCH 01/17] Initial plan From aac9780749c6c2305a76f99b8148d23974ed0a74 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 14:07:52 +0000 Subject: [PATCH 02/17] feat: register MCP servers as native tools Co-authored-by: geffzhang <439390+geffzhang@users.noreply.github.com> Agent-Logs-Url: https://github.com/geffzhang/openclaw.net/sessions/8d16cf23-fb87-42c9-91f6-93bbb91550bd --- .../Plugins/McpServerToolRegistry.cs | 570 ++++++++++++++++++ .../Plugins/NativePluginRegistry.cs | 10 + src/OpenClaw.Core/Plugins/PluginModels.cs | 25 + .../Validation/ConfigValidator.cs | 45 ++ .../RuntimeInitializationExtensions.cs | 2 + .../Composition/ToolServicesExtensions.cs | 4 + .../appsettings.Production.json | 5 +- src/OpenClaw.Gateway/appsettings.json | 12 + src/OpenClaw.Tests/ConfigValidatorTests.cs | 52 ++ .../McpServerToolRegistryTests.cs | 157 +++++ 10 files changed, 881 insertions(+), 1 deletion(-) create mode 100644 src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs create mode 100644 src/OpenClaw.Tests/McpServerToolRegistryTests.cs diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs new file mode 100644 index 0000000..0715e7a --- /dev/null +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -0,0 +1,570 @@ +using System.Diagnostics; +using System.Net.Http.Headers; +using System.Text; +using System.Text.Json; +using Microsoft.Extensions.Logging; +using OpenClaw.Core.Abstractions; +using OpenClaw.Core.Plugins; +using OpenClaw.Core.Security; + +namespace OpenClaw.Agent.Plugins; + +/// +/// Discovers tools from configured MCP servers and registers them as native OpenClaw tools. +/// +public sealed class McpServerToolRegistry : IDisposable +{ + private readonly McpPluginsConfig _config; + private readonly ILogger _logger; + private readonly List _ownedResources = []; + private readonly List _tools = []; + private bool _loaded; + + /// + /// Creates a registry for configured MCP servers. + /// + public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) + { + _config = config; + _logger = logger; + } + + /// + /// Connects to configured MCP servers and registers discovered tools into the native registry. + /// + public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) + { + var tools = await LoadAsync(ct); + nativeRegistry.RegisterOwnedResource(this); + foreach (var tool in tools) + nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); + } + + internal async Task> LoadAsync(CancellationToken ct) + { + if (_loaded) + return _tools; + + _loaded = true; + if (!_config.Enabled) + return _tools; + + foreach (var (serverId, serverConfig) in _config.Servers) + { + if (!serverConfig.Enabled) + continue; + + var session = CreateSession(serverId, serverConfig); + try + { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); + var tools = await session.LoadToolsAsync(timeoutCts.Token); + _ownedResources.Add(session); + + foreach (var tool in tools) + { + _tools.Add(new DiscoveredMcpTool( + session.PluginId, + new McpNativeTool(session, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), + session.DisplayName)); + } + } + catch + { + session.Dispose(); + throw; + } + } + + return _tools; + } + + public void Dispose() + { + foreach (var resource in _ownedResources) + resource.Dispose(); + } + + private McpServerSession CreateSession(string serverId, McpServerConfig config) + { + var displayName = string.IsNullOrWhiteSpace(config.Name) ? serverId : config.Name!; + var pluginId = $"mcp:{serverId}"; + var transport = NormalizeTransport(config); + return transport switch + { + "stdio" => new StdioMcpServerSession(serverId, pluginId, displayName, config, _logger), + "http" => new HttpMcpServerSession(serverId, pluginId, displayName, config, _logger), + _ => throw new InvalidOperationException($"Unsupported MCP transport '{config.Transport}' for server '{serverId}'.") + }; + } + + private static string NormalizeTransport(McpServerConfig config) + { + var transport = config.Transport?.Trim(); + if (string.IsNullOrWhiteSpace(transport)) + return string.IsNullOrWhiteSpace(config.Url) ? "stdio" : "http"; + + if (transport.Equals("streamable-http", StringComparison.OrdinalIgnoreCase) || + transport.Equals("streamable_http", StringComparison.OrdinalIgnoreCase)) + { + return "http"; + } + + return transport.ToLowerInvariant(); + } + + internal sealed record DiscoveredMcpTool(string PluginId, ITool Tool, string Detail); + + private sealed class McpNativeTool( + McpServerSession session, + string localName, + string remoteName, + string description, + string parameterSchema) : ITool + { + public string Name => localName; + public string Description => description; + public string ParameterSchema => parameterSchema; + + public async ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) + { + try + { + using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); + return await session.CallToolAsTextAsync(remoteName, argsDoc.RootElement.Clone(), ct); + } + catch (JsonException ex) + { + return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; + } + catch (Exception ex) + { + return $"Error: MCP tool '{localName}' failed: {ex.Message}"; + } + } + } + + private abstract class McpServerSession : IDisposable + { + protected readonly string ServerId; + private readonly string? _toolNamePrefix; + + protected McpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) + { + ServerId = serverId; + PluginId = pluginId; + DisplayName = displayName; + Config = config; + Logger = logger; + _toolNamePrefix = config.ToolNamePrefix; + } + + protected McpServerConfig Config { get; } + protected ILogger Logger { get; } + public string PluginId { get; } + public string DisplayName { get; } + + public async Task> LoadToolsAsync(CancellationToken ct) + { + await InitializeAsync(ct); + var response = await SendRequestAsync("tools/list", null, ct); + return ParseTools(response); + } + + public async Task CallToolAsTextAsync(string remoteName, JsonElement arguments, CancellationToken ct) + { + var response = await SendRequestAsync( + "tools/call", + writer => + { + writer.WriteString("name", remoteName); + writer.WritePropertyName("arguments"); + arguments.WriteTo(writer); + }, + ct); + + var isError = response.TryGetProperty("isError", out var isErrorEl) && isErrorEl.ValueKind == JsonValueKind.True; + if (response.TryGetProperty("content", out var contentEl) && contentEl.ValueKind == JsonValueKind.Array) + { + var parts = new List(); + foreach (var item in contentEl.EnumerateArray()) + { + if (item.ValueKind != JsonValueKind.Object) + continue; + + if (item.TryGetProperty("type", out var typeEl) && + typeEl.ValueKind == JsonValueKind.String && + string.Equals(typeEl.GetString(), "text", StringComparison.OrdinalIgnoreCase) && + item.TryGetProperty("text", out var textEl) && + textEl.ValueKind == JsonValueKind.String) + { + parts.Add(textEl.GetString() ?? ""); + } + } + + if (parts.Count > 0) + { + var text = string.Join("\n\n", parts); + return isError ? $"Error: {text}" : text; + } + } + + var raw = response.GetRawText(); + return isError ? $"Error: {raw}" : raw; + } + + public abstract void Dispose(); + + protected abstract Task SendRequestAsync( + string method, + Action? writeParams, + CancellationToken ct); + + protected virtual async Task InitializeAsync(CancellationToken ct) + { + await SendRequestAsync( + "initialize", + writer => + { + writer.WriteString("protocolVersion", "2025-03-26"); + writer.WriteStartObject("capabilities"); + writer.WriteEndObject(); + writer.WriteStartObject("clientInfo"); + writer.WriteString("name", "OpenClaw Gateway"); + writer.WriteString("version", "1.0.0"); + writer.WriteEndObject(); + }, + ct); + } + + protected string ResolveToolName(string remoteName) + { + var prefix = _toolNamePrefix; + if (prefix is null) + prefix = $"{SanitizePrefixPart(ServerId)}."; + + return string.IsNullOrEmpty(prefix) ? remoteName : prefix + remoteName; + } + + private IReadOnlyList ParseTools(JsonElement result) + { + if (!result.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) + throw new InvalidOperationException($"MCP server '{DisplayName}' returned an invalid tools/list response."); + + var tools = new List(); + foreach (var item in toolsEl.EnumerateArray()) + { + if (item.ValueKind != JsonValueKind.Object || + !item.TryGetProperty("name", out var nameEl) || + nameEl.ValueKind != JsonValueKind.String) + { + throw new InvalidOperationException($"MCP server '{DisplayName}' returned a tool entry without a valid name."); + } + + var remoteName = nameEl.GetString() ?? ""; + if (string.IsNullOrWhiteSpace(remoteName)) + throw new InvalidOperationException($"MCP server '{DisplayName}' returned a tool entry with an empty name."); + var localName = ResolveToolName(remoteName); + var description = item.TryGetProperty("description", out var descriptionEl) && descriptionEl.ValueKind == JsonValueKind.String + ? $"{descriptionEl.GetString()} (from MCP server '{DisplayName}')" + : $"MCP tool '{remoteName}' from server '{DisplayName}'."; + var inputSchema = item.TryGetProperty("inputSchema", out var schemaEl) ? schemaEl.GetRawText() : "{}"; + tools.Add(new McpToolDescriptor(localName, remoteName, description, inputSchema)); + } + + Logger.LogInformation("MCP server enabled: {ServerId} ({DisplayName}) with {ToolCount} tool(s)", + ServerId, DisplayName, tools.Count); + return tools; + } + + protected static string BuildRequestJson(string requestId, string method, Action? writeParams) + { + using var ms = new MemoryStream(); + using (var writer = new Utf8JsonWriter(ms)) + { + writer.WriteStartObject(); + writer.WriteString("jsonrpc", "2.0"); + writer.WriteString("id", requestId); + writer.WriteString("method", method); + writer.WritePropertyName("params"); + writer.WriteStartObject(); + writeParams?.Invoke(writer); + writer.WriteEndObject(); + writer.WriteEndObject(); + } + + return Encoding.UTF8.GetString(ms.ToArray()); + } + + protected static JsonElement ParseResponse(string payload, string requestId) + { + using var responseDoc = JsonDocument.Parse(payload); + var root = responseDoc.RootElement; + if (root.ValueKind != JsonValueKind.Object) + throw new InvalidOperationException("MCP server returned a non-object JSON-RPC response."); + + if (!root.TryGetProperty("id", out var idEl) || !IdsMatch(idEl, requestId)) + throw new InvalidOperationException("MCP server returned an unexpected JSON-RPC response id."); + + if (root.TryGetProperty("error", out var errorEl) && errorEl.ValueKind == JsonValueKind.Object) + { + var code = errorEl.TryGetProperty("code", out var codeEl) && codeEl.ValueKind == JsonValueKind.Number ? codeEl.GetInt32() : 0; + var message = errorEl.TryGetProperty("message", out var messageEl) && messageEl.ValueKind == JsonValueKind.String + ? messageEl.GetString() + : "Unknown MCP error."; + throw new InvalidOperationException($"MCP request failed ({code}): {message}"); + } + + if (!root.TryGetProperty("result", out var resultEl)) + throw new InvalidOperationException("MCP server response did not contain a result."); + + return resultEl.Clone(); + } + + private static bool IdsMatch(JsonElement responseId, string requestId) + => responseId.ValueKind switch + { + JsonValueKind.String => string.Equals(responseId.GetString(), requestId, StringComparison.Ordinal), + JsonValueKind.Number => string.Equals(responseId.GetRawText(), requestId, StringComparison.Ordinal), + _ => false + }; + + private static string SanitizePrefixPart(string value) + { + if (string.IsNullOrWhiteSpace(value)) + return "mcp"; + + var sb = new StringBuilder(value.Length); + foreach (var ch in value) + { + if (char.IsLetterOrDigit(ch) || ch is '_' or '-' or '.') + sb.Append(char.ToLowerInvariant(ch)); + else + sb.Append('_'); + } + + return sb.Length == 0 ? "mcp" : sb.ToString(); + } + } + + private sealed record McpToolDescriptor(string LocalName, string RemoteName, string Description, string InputSchemaText); + + private sealed class HttpMcpServerSession : McpServerSession + { + private readonly HttpClient _httpClient; + private long _requestId; + + public HttpMcpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) + : base(serverId, pluginId, displayName, config, logger) + { + if (string.IsNullOrWhiteSpace(config.Url)) + throw new InvalidOperationException($"MCP server '{serverId}' requires Url for HTTP transport."); + + _httpClient = new HttpClient + { + BaseAddress = new Uri(config.Url, UriKind.Absolute) + }; + + _httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); + foreach (var (headerName, rawValue) in config.Headers) + { + var value = SecretResolver.Resolve(rawValue) ?? rawValue; + _httpClient.DefaultRequestHeaders.TryAddWithoutValidation(headerName, value); + } + } + + public override void Dispose() => _httpClient.Dispose(); + + protected override async Task SendRequestAsync( + string method, + Action? writeParams, + CancellationToken ct) + { + var requestId = Interlocked.Increment(ref _requestId).ToString(System.Globalization.CultureInfo.InvariantCulture); + var payload = BuildRequestJson(requestId, method, writeParams); + using var request = new HttpRequestMessage(HttpMethod.Post, "") + { + Content = new StringContent(payload, Encoding.UTF8, "application/json") + }; + + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); + using var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, timeoutCts.Token); + response.EnsureSuccessStatusCode(); + var responsePayload = await response.Content.ReadAsStringAsync(timeoutCts.Token); + return ParseResponse(responsePayload, requestId); + } + } + + private sealed class StdioMcpServerSession : McpServerSession + { + private readonly Process _process; + private readonly Stream _stdin; + private readonly Stream _stdout; + private readonly SemaphoreSlim _mutex = new(1, 1); + private long _requestId; + + public StdioMcpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) + : base(serverId, pluginId, displayName, config, logger) + { + if (string.IsNullOrWhiteSpace(config.Command)) + throw new InvalidOperationException($"MCP server '{serverId}' requires Command for stdio transport."); + + var startInfo = new ProcessStartInfo + { + FileName = config.Command, + WorkingDirectory = string.IsNullOrWhiteSpace(config.WorkingDirectory) ? Environment.CurrentDirectory : config.WorkingDirectory, + UseShellExecute = false, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + CreateNoWindow = true + }; + + foreach (var argument in config.Arguments) + startInfo.ArgumentList.Add(argument); + foreach (var (name, rawValue) in config.Environment) + startInfo.Environment[name] = SecretResolver.Resolve(rawValue) ?? rawValue; + + _process = Process.Start(startInfo) + ?? throw new InvalidOperationException($"Failed to start MCP server '{serverId}' using command '{config.Command}'."); + _stdin = _process.StandardInput.BaseStream; + _stdout = _process.StandardOutput.BaseStream; + + _ = Task.Run(async () => + { + while (!_process.HasExited) + { + var line = await _process.StandardError.ReadLineAsync(); + if (line is null) + break; + if (!string.IsNullOrWhiteSpace(line)) + Logger.LogWarning("MCP server {ServerId} stderr: {Line}", serverId, line); + } + }); + } + + public override void Dispose() + { + _mutex.Dispose(); + _stdin.Dispose(); + _stdout.Dispose(); + if (!_process.HasExited) + { + try + { + _process.Kill(entireProcessTree: true); + } + catch + { + } + } + _process.Dispose(); + } + + protected override async Task SendRequestAsync( + string method, + Action? writeParams, + CancellationToken ct) + { + await _mutex.WaitAsync(ct); + try + { + var requestId = Interlocked.Increment(ref _requestId).ToString(System.Globalization.CultureInfo.InvariantCulture); + var payload = BuildRequestJson(requestId, method, writeParams); + await WriteMessageAsync(payload, ct); + + while (true) + { + var message = await ReadMessageAsync(ct); + using var doc = JsonDocument.Parse(message); + if (doc.RootElement.ValueKind != JsonValueKind.Object || + !doc.RootElement.TryGetProperty("id", out var idEl) || + !idEl.ValueEquals(requestId)) + { + continue; + } + + return ParseResponse(message, requestId); + } + } + finally + { + _mutex.Release(); + } + } + + private async Task WriteMessageAsync(string payload, CancellationToken ct) + { + var bytes = Encoding.UTF8.GetBytes(payload); + var header = Encoding.ASCII.GetBytes($"Content-Length: {bytes.Length}\r\n\r\n"); + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); + await _stdin.WriteAsync(header, timeoutCts.Token); + await _stdin.WriteAsync(bytes, timeoutCts.Token); + await _stdin.FlushAsync(timeoutCts.Token); + } + + private async Task ReadMessageAsync(CancellationToken ct) + { + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); + var contentLength = await ReadHeadersAsync(timeoutCts.Token); + var buffer = new byte[contentLength]; + var offset = 0; + while (offset < contentLength) + { + var read = await _stdout.ReadAsync(buffer.AsMemory(offset, contentLength - offset), timeoutCts.Token); + if (read == 0) + throw new InvalidOperationException($"MCP server '{ServerId}' closed its stdout unexpectedly."); + offset += read; + } + + return Encoding.UTF8.GetString(buffer); + } + + private async Task ReadHeadersAsync(CancellationToken ct) + { + var contentLength = -1; + while (true) + { + var line = await ReadLineAsync(ct); + if (line.Length == 0) + break; + + const string headerName = "Content-Length:"; + if (line.StartsWith(headerName, StringComparison.OrdinalIgnoreCase) && + int.TryParse(line[headerName.Length..].Trim(), out var parsed)) + { + contentLength = parsed; + } + } + + if (contentLength < 0) + throw new InvalidOperationException($"MCP server '{ServerId}' did not send a Content-Length header."); + + return contentLength; + } + + private async Task ReadLineAsync(CancellationToken ct) + { + var bytes = new List(); + while (true) + { + var next = new byte[1]; + var read = await _stdout.ReadAsync(next.AsMemory(0, 1), ct); + if (read == 0) + throw new InvalidOperationException($"MCP server '{ServerId}' closed its stdout unexpectedly."); + + if (next[0] == (byte)'\n') + break; + + if (next[0] != (byte)'\r') + bytes.Add(next[0]); + } + + return Encoding.ASCII.GetString(bytes.ToArray()); + } + } +} diff --git a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs index 4aeecd0..09b5e0d 100644 --- a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs @@ -14,6 +14,7 @@ public sealed class NativePluginRegistry : IDisposable { private readonly List _tools = []; private readonly Dictionary _nativeToolIds = new(StringComparer.Ordinal); + private readonly List _ownedResources = []; private readonly ILogger _logger; public NativePluginRegistry(NativePluginsConfig config, ILogger logger, ToolingConfig? toolingConfig = null) @@ -77,6 +78,12 @@ private void RegisterTool(ITool tool, string pluginId, string? detail = null) pluginId, detail is not null ? $" ({detail})" : ""); } + public void RegisterExternalTool(ITool tool, string pluginId, string? detail = null) + => RegisterTool(tool, pluginId, detail); + + public void RegisterOwnedResource(IDisposable resource) + => _ownedResources.Add(resource); + /// /// All enabled native plugin tools. /// @@ -202,5 +209,8 @@ public void Dispose() if (tool is IDisposable d) d.Dispose(); } + + foreach (var resource in _ownedResources) + resource.Dispose(); } } diff --git a/src/OpenClaw.Core/Plugins/PluginModels.cs b/src/OpenClaw.Core/Plugins/PluginModels.cs index d307374..28cad63 100644 --- a/src/OpenClaw.Core/Plugins/PluginModels.cs +++ b/src/OpenClaw.Core/Plugins/PluginModels.cs @@ -106,10 +106,35 @@ public sealed class PluginsConfig /// Configuration for native plugin replicas. public NativePluginsConfig Native { get; set; } = new(); + /// Configuration for MCP servers exposed as native tools. + public McpPluginsConfig Mcp { get; set; } = new(); + /// Configuration for in-process dynamic .NET plugins. JIT mode only. public NativeDynamicPluginsConfig DynamicNative { get; set; } = new(); } +public sealed class McpPluginsConfig +{ + public bool Enabled { get; set; } = false; + public Dictionary Servers { get; set; } = new(StringComparer.Ordinal); +} + +public sealed class McpServerConfig +{ + public bool Enabled { get; set; } = true; + public string? Name { get; set; } + public string? Transport { get; set; } + public string? Command { get; set; } + public string[] Arguments { get; set; } = []; + public string? WorkingDirectory { get; set; } + public Dictionary Environment { get; set; } = new(StringComparer.Ordinal); + public string? Url { get; set; } + public Dictionary Headers { get; set; } = new(StringComparer.OrdinalIgnoreCase); + public string? ToolNamePrefix { get; set; } + public int StartupTimeoutSeconds { get; set; } = 15; + public int RequestTimeoutSeconds { get; set; } = 60; +} + /// /// Configuration for native (C#) replicas of popular OpenClaw plugins. /// Each property matches the canonical plugin id. diff --git a/src/OpenClaw.Core/Validation/ConfigValidator.cs b/src/OpenClaw.Core/Validation/ConfigValidator.cs index b85af46..7b3b779 100644 --- a/src/OpenClaw.Core/Validation/ConfigValidator.cs +++ b/src/OpenClaw.Core/Validation/ConfigValidator.cs @@ -166,6 +166,36 @@ public static IReadOnlyList Validate(Models.GatewayConfig config) if (runtimeOrchestrator is not (RuntimeOrchestrator.Native or RuntimeOrchestrator.Maf)) errors.Add("Runtime.Orchestrator must be 'native' or 'maf'."); + // MCP plugin servers + if (config.Plugins.Mcp.Enabled) + { + foreach (var (serverId, server) in config.Plugins.Mcp.Servers) + { + var transport = NormalizeMcpTransport(server); + if (transport is not ("stdio" or "http")) + { + errors.Add($"Plugins.Mcp.Servers.{serverId}.Transport must be 'stdio' or 'http'."); + continue; + } + + if (server.StartupTimeoutSeconds < 1) + errors.Add($"Plugins.Mcp.Servers.{serverId}.StartupTimeoutSeconds must be >= 1 (got {server.StartupTimeoutSeconds})."); + if (server.RequestTimeoutSeconds < 1) + errors.Add($"Plugins.Mcp.Servers.{serverId}.RequestTimeoutSeconds must be >= 1 (got {server.RequestTimeoutSeconds})."); + + if (transport == "stdio") + { + if (string.IsNullOrWhiteSpace(server.Command)) + errors.Add($"Plugins.Mcp.Servers.{serverId}.Command must be set when Transport='stdio'."); + } + else if (!Uri.TryCreate(server.Url, UriKind.Absolute, out var url) || + (url.Scheme != Uri.UriSchemeHttp && url.Scheme != Uri.UriSchemeHttps)) + { + errors.Add($"Plugins.Mcp.Servers.{serverId}.Url must be an absolute http(s) URL when Transport='http'."); + } + } + } + // Channels if (config.Channels.Sms.Twilio.MaxInboundChars < 1) errors.Add($"Channels.Sms.Twilio.MaxInboundChars must be >= 1 (got {config.Channels.Sms.Twilio.MaxInboundChars})."); @@ -326,4 +356,19 @@ private static void ValidateDmPolicy(string field, string? value, ICollection InitializeOpenClawRuntimeAsync( var pipeline = app.Services.GetRequiredService(); var wsChannel = app.Services.GetRequiredService(); var nativeRegistry = app.Services.GetRequiredService(); + var mcpRegistry = app.Services.GetRequiredService(); var runtimeDiagnostics = new Dictionary>(StringComparer.Ordinal); var dynamicProviderOwners = new HashSet(StringComparer.Ordinal); var blockedPluginIds = pluginHealth.GetBlockedPluginIds(); @@ -103,6 +104,7 @@ public static async Task InitializeOpenClawRuntimeAsync( loggerFactory.CreateLogger()); var builtInTools = CreateBuiltInTools(config, memoryStore, sessionManager, pipeline, startup.WorkspacePath); + await mcpRegistry.RegisterToolsAsync(nativeRegistry, app.Lifetime.ApplicationStopping); LlmClientFactory.ResetDynamicProviders(); try { diff --git a/src/OpenClaw.Gateway/Composition/ToolServicesExtensions.cs b/src/OpenClaw.Gateway/Composition/ToolServicesExtensions.cs index e16efa0..4172f51 100644 --- a/src/OpenClaw.Gateway/Composition/ToolServicesExtensions.cs +++ b/src/OpenClaw.Gateway/Composition/ToolServicesExtensions.cs @@ -12,6 +12,10 @@ public static IServiceCollection AddOpenClawToolServices(this IServiceCollection startup.Config.Plugins.Native, sp.GetRequiredService().CreateLogger(), startup.Config.Tooling)); + services.AddSingleton(sp => + new McpServerToolRegistry( + startup.Config.Plugins.Mcp, + sp.GetRequiredService().CreateLogger())); return services; } diff --git a/src/OpenClaw.Gateway/appsettings.Production.json b/src/OpenClaw.Gateway/appsettings.Production.json index 7d8bb81..dca5160 100644 --- a/src/OpenClaw.Gateway/appsettings.Production.json +++ b/src/OpenClaw.Gateway/appsettings.Production.json @@ -68,7 +68,10 @@ }, "Plugins": { - "Enabled": false + "Enabled": false, + "Mcp": { + "Enabled": false + } }, "MaxConcurrentSessions": 128, diff --git a/src/OpenClaw.Gateway/appsettings.json b/src/OpenClaw.Gateway/appsettings.json index 6194a6c..64d83cd 100644 --- a/src/OpenClaw.Gateway/appsettings.json +++ b/src/OpenClaw.Gateway/appsettings.json @@ -159,6 +159,18 @@ "ImapOperationTimeoutSeconds": 0, "MaxResponseLinesPerCommand": 10000 } + }, + "Mcp": { + "Enabled": false, + "Servers": { + "example": { + "Enabled": false, + "Name": "example", + "Transport": "http", + "Url": "http://127.0.0.1:3001/mcp", + "ToolNamePrefix": "example." + } + } } }, "Channels": { diff --git a/src/OpenClaw.Tests/ConfigValidatorTests.cs b/src/OpenClaw.Tests/ConfigValidatorTests.cs index 22e4823..4c88f6f 100644 --- a/src/OpenClaw.Tests/ConfigValidatorTests.cs +++ b/src/OpenClaw.Tests/ConfigValidatorTests.cs @@ -172,6 +172,58 @@ public void Validate_InvalidRuntimeOrchestrator_ReturnsError() Assert.Contains(errors, e => e.Contains("Runtime.Orchestrator", StringComparison.Ordinal)); } + [Fact] + public void Validate_McpHttpServerWithoutUrl_ReturnsError() + { + var config = new GatewayConfig + { + Plugins = new PluginsConfig + { + Mcp = new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = "" + } + } + } + } + }; + + var errors = ConfigValidator.Validate(config); + Assert.Contains(errors, e => e.Contains("Plugins.Mcp.Servers.demo.Url", StringComparison.Ordinal)); + } + + [Fact] + public void Validate_McpStdioServerWithoutCommand_ReturnsError() + { + var config = new GatewayConfig + { + Plugins = new PluginsConfig + { + Mcp = new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "stdio", + Command = "" + } + } + } + } + }; + + var errors = ConfigValidator.Validate(config); + Assert.Contains(errors, e => e.Contains("Plugins.Mcp.Servers.demo.Command", StringComparison.Ordinal)); + } + [Fact] public void Validate_OpenSandboxProviderWithoutEndpoint_ReturnsError() { diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs new file mode 100644 index 0000000..23d22eb --- /dev/null +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -0,0 +1,157 @@ +using System.Text; +using System.Text.Json; +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Logging.Abstractions; +using OpenClaw.Agent.Plugins; +using OpenClaw.Core.Models; +using OpenClaw.Core.Plugins; +using Xunit; + +namespace OpenClaw.Tests; + +public sealed class McpServerToolRegistryTests : IAsyncDisposable +{ + private readonly List _apps = []; + + [Fact] + public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() + { + var (serverUrl, calls) = await StartMcpServerAsync(); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + var tool = Assert.Single(nativeRegistry.Tools); + Assert.Equal("demo.echo", tool.Name); + Assert.Contains("Demo echo tool", tool.Description, StringComparison.Ordinal); + Assert.Equal("demo:hello", await tool.ExecuteAsync("""{"text":"hello"}""", CancellationToken.None)); + Assert.Equal(1, calls.InitializeCalls); + Assert.Equal(1, calls.ListCalls); + Assert.Equal(1, calls.CallCalls); + } + + public async ValueTask DisposeAsync() + { + foreach (var app in _apps) + await app.DisposeAsync(); + } + + private async Task<(string ServerUrl, McpCallTracker Tracker)> StartMcpServerAsync() + { + var tracker = new McpCallTracker(); + var builder = WebApplication.CreateSlimBuilder(); + builder.WebHost.ConfigureKestrel(options => options.ListenLocalhost(0)); + var app = builder.Build(); + app.MapPost("/", async context => + { + using var document = await JsonDocument.ParseAsync(context.Request.Body, cancellationToken: context.RequestAborted); + var method = document.RootElement.GetProperty("method").GetString(); + var id = document.RootElement.GetProperty("id").GetString()!; + + var response = method switch + { + "initialize" => BuildResponse(id, writer => + { + tracker.InitializeCalls++; + writer.WriteStartObject("capabilities"); + writer.WriteStartObject("tools"); + writer.WriteBoolean("listChanged", false); + writer.WriteEndObject(); + writer.WriteStartObject("resources"); + writer.WriteBoolean("listChanged", false); + writer.WriteBoolean("supportsTemplates", false); + writer.WriteEndObject(); + writer.WriteStartObject("prompts"); + writer.WriteBoolean("listChanged", false); + writer.WriteEndObject(); + writer.WriteEndObject(); + writer.WriteStartObject("serverInfo"); + writer.WriteString("name", "demo"); + writer.WriteString("version", "1.0.0"); + writer.WriteEndObject(); + }), + "tools/list" => BuildResponse(id, writer => + { + tracker.ListCalls++; + writer.WriteStartArray("tools"); + writer.WriteStartObject(); + writer.WriteString("name", "echo"); + writer.WriteString("description", "Demo echo tool"); + writer.WriteStartObject("inputSchema"); + writer.WriteString("type", "object"); + writer.WriteStartObject("properties"); + writer.WriteStartObject("text"); + writer.WriteString("type", "string"); + writer.WriteEndObject(); + writer.WriteEndObject(); + writer.WriteStartArray("required"); + writer.WriteStringValue("text"); + writer.WriteEndArray(); + writer.WriteEndObject(); + writer.WriteEndObject(); + writer.WriteEndArray(); + }), + "tools/call" => BuildResponse(id, writer => + { + tracker.CallCalls++; + var text = document.RootElement.GetProperty("params").GetProperty("arguments").GetProperty("text").GetString(); + writer.WriteStartArray("content"); + writer.WriteStartObject(); + writer.WriteString("type", "text"); + writer.WriteString("text", $"demo:{text}"); + writer.WriteEndObject(); + writer.WriteEndArray(); + writer.WriteBoolean("isError", false); + }), + _ => throw new InvalidOperationException($"Unexpected MCP method '{method}'.") + }; + + context.Response.ContentType = "application/json"; + await context.Response.WriteAsync(response, context.RequestAborted); + }); + + await app.StartAsync(); + _apps.Add(app); + var address = app.Urls.Single(); + return ($"{address.TrimEnd('/')}/", tracker); + } + + private static string BuildResponse(string id, Action writeResult) + { + using var ms = new MemoryStream(); + using (var writer = new Utf8JsonWriter(ms)) + { + writer.WriteStartObject(); + writer.WriteString("jsonrpc", "2.0"); + writer.WriteString("id", id); + writer.WritePropertyName("result"); + writer.WriteStartObject(); + writeResult(writer); + writer.WriteEndObject(); + writer.WriteEndObject(); + } + + return Encoding.UTF8.GetString(ms.ToArray()); + } + + private sealed class McpCallTracker + { + public int InitializeCalls { get; set; } + public int ListCalls { get; set; } + public int CallCalls { get; set; } + } +} From 55f960206cc6238c8528b1d670d4df05e3bed64e Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 10:06:21 +0800 Subject: [PATCH 03/17] Use ModelContextProtocol client for MCP servers Add ModelContextProtocol dependency and refactor McpServerToolRegistry to use McpClient and transport implementations instead of custom McpServerSession types. Introduces transport creation, header and environment variable resolution, tool name sanitization, and centralized tool loading via the ModelContextProtocol client. Update disposal to dispose clients and simplify tool execution to use the client's CallTool API. Tests updated to exercise the new HTTP transport, verify header/secret resolution, and use an embedded MCP server with DemoMcpTools. --- src/OpenClaw.Agent/OpenClaw.Agent.csproj | 1 + .../Plugins/McpServerToolRegistry.cs | 598 +++++------------- .../McpServerToolRegistryTests.cs | 219 ++++--- 3 files changed, 308 insertions(+), 510 deletions(-) diff --git a/src/OpenClaw.Agent/OpenClaw.Agent.csproj b/src/OpenClaw.Agent/OpenClaw.Agent.csproj index c5890fb..eb8b3a9 100644 --- a/src/OpenClaw.Agent/OpenClaw.Agent.csproj +++ b/src/OpenClaw.Agent/OpenClaw.Agent.csproj @@ -14,6 +14,7 @@ + diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 0715e7a..912a3d1 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -3,6 +3,9 @@ using System.Text; using System.Text.Json; using Microsoft.Extensions.Logging; +using ModelContextProtocol; +using ModelContextProtocol.Client; +using ModelContextProtocol.Protocol; using OpenClaw.Core.Abstractions; using OpenClaw.Core.Plugins; using OpenClaw.Core.Security; @@ -18,6 +21,7 @@ public sealed class McpServerToolRegistry : IDisposable private readonly ILogger _logger; private readonly List _ownedResources = []; private readonly List _tools = []; + private readonly List _clients = []; private bool _loaded; /// @@ -54,25 +58,32 @@ internal async Task> LoadAsync(CancellationToke if (!serverConfig.Enabled) continue; - var session = CreateSession(serverId, serverConfig); + var transport = CreateTransport(serverId, serverConfig); + McpClient? client = null; try { using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); - var tools = await session.LoadToolsAsync(timeoutCts.Token); - _ownedResources.Add(session); + client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); + _clients.Add(client); + + var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; + var pluginId = $"mcp:{serverId}"; + + var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, timeoutCts.Token); foreach (var tool in tools) { _tools.Add(new DiscoveredMcpTool( - session.PluginId, - new McpNativeTool(session, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), - session.DisplayName)); + pluginId, + new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), + displayName)); } } catch { - session.Dispose(); + if (client is IDisposable disposable) + disposable.Dispose(); throw; } } @@ -80,491 +91,216 @@ internal async Task> LoadAsync(CancellationToke return _tools; } - public void Dispose() + private async Task> LoadToolsFromClientAsync( + McpClient client, + string serverId, + string pluginId, + string displayName, + McpServerConfig config, + CancellationToken ct) { - foreach (var resource in _ownedResources) - resource.Dispose(); + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(config.RequestTimeoutSeconds)); + var response = await client.ListToolsAsync(cancellationToken: timeoutCts.Token); + + var tools = new List(); + foreach (var tool in response) + { + var remoteName = tool.Name; + if (string.IsNullOrWhiteSpace(remoteName)) + throw new InvalidOperationException($"MCP server '{displayName}' returned a tool entry with an empty name."); + + var localName = ResolveToolName(serverId, config.ToolNamePrefix, remoteName); + var description = !string.IsNullOrWhiteSpace(tool.Description) + ? $"{tool.Description} (from MCP server '{displayName}')" + : $"MCP tool '{remoteName}' from server '{displayName}'."; + var inputSchema = "{}"; + tools.Add(new McpToolDescriptor(localName, remoteName, description, inputSchema)); + } + + _logger.LogInformation("MCP server enabled: {ServerId} ({DisplayName}) with {ToolCount} tool(s)", + serverId, displayName, tools.Count); + return tools; } - private McpServerSession CreateSession(string serverId, McpServerConfig config) + private static string ResolveToolName(string serverId, string? toolNamePrefix, string remoteName) { - var displayName = string.IsNullOrWhiteSpace(config.Name) ? serverId : config.Name!; - var pluginId = $"mcp:{serverId}"; - var transport = NormalizeTransport(config); - return transport switch - { - "stdio" => new StdioMcpServerSession(serverId, pluginId, displayName, config, _logger), - "http" => new HttpMcpServerSession(serverId, pluginId, displayName, config, _logger), - _ => throw new InvalidOperationException($"Unsupported MCP transport '{config.Transport}' for server '{serverId}'.") - }; + var prefix = toolNamePrefix; + if (prefix is null) + prefix = $"{SanitizePrefixPart(serverId)}."; + + return string.IsNullOrEmpty(prefix) ? remoteName : prefix + remoteName; } - private static string NormalizeTransport(McpServerConfig config) + private static string SanitizePrefixPart(string value) { - var transport = config.Transport?.Trim(); - if (string.IsNullOrWhiteSpace(transport)) - return string.IsNullOrWhiteSpace(config.Url) ? "stdio" : "http"; + if (string.IsNullOrWhiteSpace(value)) + return "mcp"; - if (transport.Equals("streamable-http", StringComparison.OrdinalIgnoreCase) || - transport.Equals("streamable_http", StringComparison.OrdinalIgnoreCase)) + var sb = new StringBuilder(value.Length); + foreach (var ch in value) { - return "http"; + if (char.IsLetterOrDigit(ch) || ch is '_' or '-' or '.') + sb.Append(char.ToLowerInvariant(ch)); + else + sb.Append('_'); } - return transport.ToLowerInvariant(); + return sb.Length == 0 ? "mcp" : sb.ToString(); } - internal sealed record DiscoveredMcpTool(string PluginId, ITool Tool, string Detail); - - private sealed class McpNativeTool( - McpServerSession session, - string localName, - string remoteName, - string description, - string parameterSchema) : ITool + public void Dispose() { - public string Name => localName; - public string Description => description; - public string ParameterSchema => parameterSchema; - - public async ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) + foreach (var resource in _ownedResources) + resource.Dispose(); + foreach (var client in _clients) { try { - using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); - return await session.CallToolAsTextAsync(remoteName, argsDoc.RootElement.Clone(), ct); - } - catch (JsonException ex) - { - return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; + (client as IDisposable)?.Dispose(); + (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); } - catch (Exception ex) + catch { - return $"Error: MCP tool '{localName}' failed: {ex.Message}"; } } } - private abstract class McpServerSession : IDisposable - { - protected readonly string ServerId; - private readonly string? _toolNamePrefix; - protected McpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) - { - ServerId = serverId; - PluginId = pluginId; - DisplayName = displayName; - Config = config; - Logger = logger; - _toolNamePrefix = config.ToolNamePrefix; - } - - protected McpServerConfig Config { get; } - protected ILogger Logger { get; } - public string PluginId { get; } - public string DisplayName { get; } - - public async Task> LoadToolsAsync(CancellationToken ct) - { - await InitializeAsync(ct); - var response = await SendRequestAsync("tools/list", null, ct); - return ParseTools(response); - } - - public async Task CallToolAsTextAsync(string remoteName, JsonElement arguments, CancellationToken ct) - { - var response = await SendRequestAsync( - "tools/call", - writer => - { - writer.WriteString("name", remoteName); - writer.WritePropertyName("arguments"); - arguments.WriteTo(writer); - }, - ct); - - var isError = response.TryGetProperty("isError", out var isErrorEl) && isErrorEl.ValueKind == JsonValueKind.True; - if (response.TryGetProperty("content", out var contentEl) && contentEl.ValueKind == JsonValueKind.Array) - { - var parts = new List(); - foreach (var item in contentEl.EnumerateArray()) - { - if (item.ValueKind != JsonValueKind.Object) - continue; - - if (item.TryGetProperty("type", out var typeEl) && - typeEl.ValueKind == JsonValueKind.String && - string.Equals(typeEl.GetString(), "text", StringComparison.OrdinalIgnoreCase) && - item.TryGetProperty("text", out var textEl) && - textEl.ValueKind == JsonValueKind.String) - { - parts.Add(textEl.GetString() ?? ""); - } - } - - if (parts.Count > 0) - { - var text = string.Join("\n\n", parts); - return isError ? $"Error: {text}" : text; - } - } - var raw = response.GetRawText(); - return isError ? $"Error: {raw}" : raw; - } - - public abstract void Dispose(); - - protected abstract Task SendRequestAsync( - string method, - Action? writeParams, - CancellationToken ct); + private static string NormalizeTransport(McpServerConfig config) + { + var transport = config.Transport?.Trim(); + if (string.IsNullOrWhiteSpace(transport)) + return string.IsNullOrWhiteSpace(config.Url) ? "stdio" : "http"; - protected virtual async Task InitializeAsync(CancellationToken ct) + if (transport.Equals("streamable-http", StringComparison.OrdinalIgnoreCase) || + transport.Equals("streamable_http", StringComparison.OrdinalIgnoreCase)) { - await SendRequestAsync( - "initialize", - writer => - { - writer.WriteString("protocolVersion", "2025-03-26"); - writer.WriteStartObject("capabilities"); - writer.WriteEndObject(); - writer.WriteStartObject("clientInfo"); - writer.WriteString("name", "OpenClaw Gateway"); - writer.WriteString("version", "1.0.0"); - writer.WriteEndObject(); - }, - ct); + return "http"; } - protected string ResolveToolName(string remoteName) - { - var prefix = _toolNamePrefix; - if (prefix is null) - prefix = $"{SanitizePrefixPart(ServerId)}."; - - return string.IsNullOrEmpty(prefix) ? remoteName : prefix + remoteName; - } + return transport.ToLowerInvariant(); + } - private IReadOnlyList ParseTools(JsonElement result) + private static IClientTransport CreateTransport(string serverId, McpServerConfig config) + { + var transport = NormalizeTransport(config); + return transport switch { - if (!result.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) - throw new InvalidOperationException($"MCP server '{DisplayName}' returned an invalid tools/list response."); - - var tools = new List(); - foreach (var item in toolsEl.EnumerateArray()) + "stdio" => new StdioClientTransport(new StdioClientTransportOptions { - if (item.ValueKind != JsonValueKind.Object || - !item.TryGetProperty("name", out var nameEl) || - nameEl.ValueKind != JsonValueKind.String) - { - throw new InvalidOperationException($"MCP server '{DisplayName}' returned a tool entry without a valid name."); - } - - var remoteName = nameEl.GetString() ?? ""; - if (string.IsNullOrWhiteSpace(remoteName)) - throw new InvalidOperationException($"MCP server '{DisplayName}' returned a tool entry with an empty name."); - var localName = ResolveToolName(remoteName); - var description = item.TryGetProperty("description", out var descriptionEl) && descriptionEl.ValueKind == JsonValueKind.String - ? $"{descriptionEl.GetString()} (from MCP server '{DisplayName}')" - : $"MCP tool '{remoteName}' from server '{DisplayName}'."; - var inputSchema = item.TryGetProperty("inputSchema", out var schemaEl) ? schemaEl.GetRawText() : "{}"; - tools.Add(new McpToolDescriptor(localName, remoteName, description, inputSchema)); - } - - Logger.LogInformation("MCP server enabled: {ServerId} ({DisplayName}) with {ToolCount} tool(s)", - ServerId, DisplayName, tools.Count); - return tools; - } - - protected static string BuildRequestJson(string requestId, string method, Action? writeParams) - { - using var ms = new MemoryStream(); - using (var writer = new Utf8JsonWriter(ms)) + Command = config.Command!, + Arguments = config.Arguments, + WorkingDirectory = config.WorkingDirectory, + EnvironmentVariables = ResolveEnv(config.Environment), + Name = serverId, + }), + "http" => new HttpClientTransport(new HttpClientTransportOptions { - writer.WriteStartObject(); - writer.WriteString("jsonrpc", "2.0"); - writer.WriteString("id", requestId); - writer.WriteString("method", method); - writer.WritePropertyName("params"); - writer.WriteStartObject(); - writeParams?.Invoke(writer); - writer.WriteEndObject(); - writer.WriteEndObject(); - } + Endpoint = new Uri(config.Url!), + AdditionalHeaders = ResolveHeaders(config.Headers), + Name = serverId, + }), + _ => throw new InvalidOperationException($"Unsupported MCP transport '{config.Transport}' for server '{serverId}'.") + }; + } - return Encoding.UTF8.GetString(ms.ToArray()); - } + private static Dictionary? ResolveEnv(Dictionary environment) + { + if (environment.Count == 0) + return null; - protected static JsonElement ParseResponse(string payload, string requestId) + var resolved = new Dictionary(StringComparer.Ordinal); + foreach (var (name, rawValue) in environment) { - using var responseDoc = JsonDocument.Parse(payload); - var root = responseDoc.RootElement; - if (root.ValueKind != JsonValueKind.Object) - throw new InvalidOperationException("MCP server returned a non-object JSON-RPC response."); - - if (!root.TryGetProperty("id", out var idEl) || !IdsMatch(idEl, requestId)) - throw new InvalidOperationException("MCP server returned an unexpected JSON-RPC response id."); - - if (root.TryGetProperty("error", out var errorEl) && errorEl.ValueKind == JsonValueKind.Object) - { - var code = errorEl.TryGetProperty("code", out var codeEl) && codeEl.ValueKind == JsonValueKind.Number ? codeEl.GetInt32() : 0; - var message = errorEl.TryGetProperty("message", out var messageEl) && messageEl.ValueKind == JsonValueKind.String - ? messageEl.GetString() - : "Unknown MCP error."; - throw new InvalidOperationException($"MCP request failed ({code}): {message}"); - } - - if (!root.TryGetProperty("result", out var resultEl)) - throw new InvalidOperationException("MCP server response did not contain a result."); - - return resultEl.Clone(); + var value = SecretResolver.Resolve(rawValue) ?? rawValue; + resolved[name] = value; } - private static bool IdsMatch(JsonElement responseId, string requestId) - => responseId.ValueKind switch - { - JsonValueKind.String => string.Equals(responseId.GetString(), requestId, StringComparison.Ordinal), - JsonValueKind.Number => string.Equals(responseId.GetRawText(), requestId, StringComparison.Ordinal), - _ => false - }; - - private static string SanitizePrefixPart(string value) - { - if (string.IsNullOrWhiteSpace(value)) - return "mcp"; - - var sb = new StringBuilder(value.Length); - foreach (var ch in value) - { - if (char.IsLetterOrDigit(ch) || ch is '_' or '-' or '.') - sb.Append(char.ToLowerInvariant(ch)); - else - sb.Append('_'); - } - - return sb.Length == 0 ? "mcp" : sb.ToString(); - } + return resolved; } - private sealed record McpToolDescriptor(string LocalName, string RemoteName, string Description, string InputSchemaText); - - private sealed class HttpMcpServerSession : McpServerSession + private static Dictionary? ResolveHeaders(Dictionary headers) { - private readonly HttpClient _httpClient; - private long _requestId; + if (headers.Count == 0) + return null; - public HttpMcpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) - : base(serverId, pluginId, displayName, config, logger) + var resolved = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var (name, rawValue) in headers) { - if (string.IsNullOrWhiteSpace(config.Url)) - throw new InvalidOperationException($"MCP server '{serverId}' requires Url for HTTP transport."); - - _httpClient = new HttpClient - { - BaseAddress = new Uri(config.Url, UriKind.Absolute) - }; - - _httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json")); - foreach (var (headerName, rawValue) in config.Headers) - { - var value = SecretResolver.Resolve(rawValue) ?? rawValue; - _httpClient.DefaultRequestHeaders.TryAddWithoutValidation(headerName, value); - } + var value = SecretResolver.Resolve(rawValue) ?? rawValue; + resolved[name] = value; } - public override void Dispose() => _httpClient.Dispose(); - - protected override async Task SendRequestAsync( - string method, - Action? writeParams, - CancellationToken ct) - { - var requestId = Interlocked.Increment(ref _requestId).ToString(System.Globalization.CultureInfo.InvariantCulture); - var payload = BuildRequestJson(requestId, method, writeParams); - using var request = new HttpRequestMessage(HttpMethod.Post, "") - { - Content = new StringContent(payload, Encoding.UTF8, "application/json") - }; - - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); - using var response = await _httpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, timeoutCts.Token); - response.EnsureSuccessStatusCode(); - var responsePayload = await response.Content.ReadAsStringAsync(timeoutCts.Token); - return ParseResponse(responsePayload, requestId); - } + return resolved; } - private sealed class StdioMcpServerSession : McpServerSession - { - private readonly Process _process; - private readonly Stream _stdin; - private readonly Stream _stdout; - private readonly SemaphoreSlim _mutex = new(1, 1); - private long _requestId; - - public StdioMcpServerSession(string serverId, string pluginId, string displayName, McpServerConfig config, ILogger logger) - : base(serverId, pluginId, displayName, config, logger) - { - if (string.IsNullOrWhiteSpace(config.Command)) - throw new InvalidOperationException($"MCP server '{serverId}' requires Command for stdio transport."); - - var startInfo = new ProcessStartInfo - { - FileName = config.Command, - WorkingDirectory = string.IsNullOrWhiteSpace(config.WorkingDirectory) ? Environment.CurrentDirectory : config.WorkingDirectory, - UseShellExecute = false, - RedirectStandardInput = true, - RedirectStandardOutput = true, - RedirectStandardError = true, - CreateNoWindow = true - }; - - foreach (var argument in config.Arguments) - startInfo.ArgumentList.Add(argument); - foreach (var (name, rawValue) in config.Environment) - startInfo.Environment[name] = SecretResolver.Resolve(rawValue) ?? rawValue; - - _process = Process.Start(startInfo) - ?? throw new InvalidOperationException($"Failed to start MCP server '{serverId}' using command '{config.Command}'."); - _stdin = _process.StandardInput.BaseStream; - _stdout = _process.StandardOutput.BaseStream; - - _ = Task.Run(async () => - { - while (!_process.HasExited) - { - var line = await _process.StandardError.ReadLineAsync(); - if (line is null) - break; - if (!string.IsNullOrWhiteSpace(line)) - Logger.LogWarning("MCP server {ServerId} stderr: {Line}", serverId, line); - } - }); - } + internal sealed record DiscoveredMcpTool(string PluginId, ITool Tool, string Detail); - public override void Dispose() - { - _mutex.Dispose(); - _stdin.Dispose(); - _stdout.Dispose(); - if (!_process.HasExited) - { - try - { - _process.Kill(entireProcessTree: true); - } - catch - { - } - } - _process.Dispose(); - } + private sealed class McpNativeTool( + McpClient client, + string localName, + string remoteName, + string description, + string parameterSchema) : ITool + { + public string Name => localName; + public string Description => description; + public string ParameterSchema => parameterSchema; - protected override async Task SendRequestAsync( - string method, - Action? writeParams, - CancellationToken ct) + public async ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) { - await _mutex.WaitAsync(ct); try { - var requestId = Interlocked.Increment(ref _requestId).ToString(System.Globalization.CultureInfo.InvariantCulture); - var payload = BuildRequestJson(requestId, method, writeParams); - await WriteMessageAsync(payload, ct); - - while (true) + using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); + var argsDict = new Dictionary(StringComparer.Ordinal); + foreach (var prop in argsDoc.RootElement.EnumerateObject()) { - var message = await ReadMessageAsync(ct); - using var doc = JsonDocument.Parse(message); - if (doc.RootElement.ValueKind != JsonValueKind.Object || - !doc.RootElement.TryGetProperty("id", out var idEl) || - !idEl.ValueEquals(requestId)) + object? value = null; + var v = prop.Value; + switch (v.ValueKind) { - continue; + case JsonValueKind.String: + value = v.GetString(); + break; + case JsonValueKind.Number: + value = v.TryGetInt64(out var l) ? l : v.GetDouble(); + break; + case JsonValueKind.True: + case JsonValueKind.False: + value = v.GetBoolean(); + break; + case JsonValueKind.Null: + value = null; + break; + default: + value = v.Clone(); + break; } - - return ParseResponse(message, requestId); + argsDict[prop.Name] = value; } - } - finally - { - _mutex.Release(); - } - } - - private async Task WriteMessageAsync(string payload, CancellationToken ct) - { - var bytes = Encoding.UTF8.GetBytes(payload); - var header = Encoding.ASCII.GetBytes($"Content-Length: {bytes.Length}\r\n\r\n"); - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); - await _stdin.WriteAsync(header, timeoutCts.Token); - await _stdin.WriteAsync(bytes, timeoutCts.Token); - await _stdin.FlushAsync(timeoutCts.Token); - } - - private async Task ReadMessageAsync(CancellationToken ct) - { - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromSeconds(Config.RequestTimeoutSeconds)); - var contentLength = await ReadHeadersAsync(timeoutCts.Token); - var buffer = new byte[contentLength]; - var offset = 0; - while (offset < contentLength) - { - var read = await _stdout.ReadAsync(buffer.AsMemory(offset, contentLength - offset), timeoutCts.Token); - if (read == 0) - throw new InvalidOperationException($"MCP server '{ServerId}' closed its stdout unexpectedly."); - offset += read; - } - - return Encoding.UTF8.GetString(buffer); - } - - private async Task ReadHeadersAsync(CancellationToken ct) - { - var contentLength = -1; - while (true) - { - var line = await ReadLineAsync(ct); - if (line.Length == 0) - break; - - const string headerName = "Content-Length:"; - if (line.StartsWith(headerName, StringComparison.OrdinalIgnoreCase) && - int.TryParse(line[headerName.Length..].Trim(), out var parsed)) + var response = await client.CallToolAsync(remoteName, argsDict, progress: null, cancellationToken: ct); + var parts = new List(); + foreach (var item in response.Content) { - contentLength = parsed; + if (item is TextContentBlock t) + parts.Add(t.Text ?? ""); } + var text = string.Join("\n\n", parts); + var isError = response.IsError ?? false; + return isError ? $"Error: {text}" : text; } - - if (contentLength < 0) - throw new InvalidOperationException($"MCP server '{ServerId}' did not send a Content-Length header."); - - return contentLength; - } - - private async Task ReadLineAsync(CancellationToken ct) - { - var bytes = new List(); - while (true) + catch (JsonException ex) { - var next = new byte[1]; - var read = await _stdout.ReadAsync(next.AsMemory(0, 1), ct); - if (read == 0) - throw new InvalidOperationException($"MCP server '{ServerId}' closed its stdout unexpectedly."); - - if (next[0] == (byte)'\n') - break; - - if (next[0] != (byte)'\r') - bytes.Add(next[0]); + return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; + } + catch (Exception ex) + { + return $"Error: MCP tool '{localName}' failed: {ex.Message}"; } - - return Encoding.ASCII.GetString(bytes.ToArray()); } } + + private sealed record McpToolDescriptor(string LocalName, string RemoteName, string Description, string InputSchemaText); } diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index 23d22eb..b963762 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -1,7 +1,15 @@ using System.Text; using System.Text.Json; +using System.ComponentModel; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Server.Kestrel.Core; +using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; +using ModelContextProtocol; +using ModelContextProtocol.Protocol; +using ModelContextProtocol.Server; using OpenClaw.Agent.Plugins; using OpenClaw.Core.Models; using OpenClaw.Core.Plugins; @@ -39,9 +47,55 @@ public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() Assert.Equal("demo.echo", tool.Name); Assert.Contains("Demo echo tool", tool.Description, StringComparison.Ordinal); Assert.Equal("demo:hello", await tool.ExecuteAsync("""{"text":"hello"}""", CancellationToken.None)); - Assert.Equal(1, calls.InitializeCalls); - Assert.Equal(1, calls.ListCalls); - Assert.Equal(1, calls.CallCalls); + Assert.True(calls.InitializeCalls >= 1); + Assert.True(calls.ListCalls >= 1); + Assert.True(calls.CallCalls >= 1); + } + + [Fact] + public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() + { + // Set up environment variable for testing + Environment.SetEnvironmentVariable("TEST_AUTH_TOKEN", "secret-token-123"); + try + { + var (serverUrl, calls, receivedHeaders) = await StartMcpServerWithHeaderCheckAsync(); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl, + Headers = new Dictionary(StringComparer.Ordinal) + { + ["Authorization"] = "env:TEST_AUTH_TOKEN", + ["X-Custom-Header"] = "raw:literal-value", + ["X-Direct-Value"] = "direct-value" + } + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + // Verify headers were resolved and sent correctly + Assert.True(receivedHeaders.ContainsKey("Authorization")); + Assert.Equal("secret-token-123", receivedHeaders["Authorization"]); + Assert.True(receivedHeaders.ContainsKey("X-Custom-Header")); + Assert.Equal("literal-value", receivedHeaders["X-Custom-Header"]); + Assert.True(receivedHeaders.ContainsKey("X-Direct-Value")); + Assert.Equal("direct-value", receivedHeaders["X-Direct-Value"]); + } + finally + { + Environment.SetEnvironmentVariable("TEST_AUTH_TOKEN", null); + } } public async ValueTask DisposeAsync() @@ -54,98 +108,97 @@ public async ValueTask DisposeAsync() { var tracker = new McpCallTracker(); var builder = WebApplication.CreateSlimBuilder(); - builder.WebHost.ConfigureKestrel(options => options.ListenLocalhost(0)); + builder.WebHost.UseUrls("http://127.0.0.1:0"); + builder.Services.AddSingleton(tracker); + builder.Services.AddMcpServer(options => + { + options.ServerInfo = new Implementation + { + Name = "demo", + Version = "1.0.0" + }; + }) + .WithHttpTransport(options => { options.Stateless = true; }) + .WithTools(); var app = builder.Build(); - app.MapPost("/", async context => + app.Use(async (context, next) => { - using var document = await JsonDocument.ParseAsync(context.Request.Body, cancellationToken: context.RequestAborted); - var method = document.RootElement.GetProperty("method").GetString(); - var id = document.RootElement.GetProperty("id").GetString()!; + await TrackMcpMethodAsync(context, tracker); + await next(); + }); + app.MapMcp("/mcp"); - var response = method switch + await app.StartAsync(); + _apps.Add(app); + var address = app.Urls.Single(); + return ($"{address.TrimEnd('/')}/mcp", tracker); + } + + private async Task<(string ServerUrl, McpCallTracker Tracker, Dictionary ReceivedHeaders)> StartMcpServerWithHeaderCheckAsync() + { + var tracker = new McpCallTracker(); + var receivedHeaders = new Dictionary(StringComparer.OrdinalIgnoreCase); + var builder = WebApplication.CreateSlimBuilder(); + builder.WebHost.UseUrls("http://127.0.0.1:0"); + builder.Services.AddSingleton(tracker); + builder.Services.AddMcpServer(options => { - "initialize" => BuildResponse(id, writer => - { - tracker.InitializeCalls++; - writer.WriteStartObject("capabilities"); - writer.WriteStartObject("tools"); - writer.WriteBoolean("listChanged", false); - writer.WriteEndObject(); - writer.WriteStartObject("resources"); - writer.WriteBoolean("listChanged", false); - writer.WriteBoolean("supportsTemplates", false); - writer.WriteEndObject(); - writer.WriteStartObject("prompts"); - writer.WriteBoolean("listChanged", false); - writer.WriteEndObject(); - writer.WriteEndObject(); - writer.WriteStartObject("serverInfo"); - writer.WriteString("name", "demo"); - writer.WriteString("version", "1.0.0"); - writer.WriteEndObject(); - }), - "tools/list" => BuildResponse(id, writer => + options.ServerInfo = new Implementation { - tracker.ListCalls++; - writer.WriteStartArray("tools"); - writer.WriteStartObject(); - writer.WriteString("name", "echo"); - writer.WriteString("description", "Demo echo tool"); - writer.WriteStartObject("inputSchema"); - writer.WriteString("type", "object"); - writer.WriteStartObject("properties"); - writer.WriteStartObject("text"); - writer.WriteString("type", "string"); - writer.WriteEndObject(); - writer.WriteEndObject(); - writer.WriteStartArray("required"); - writer.WriteStringValue("text"); - writer.WriteEndArray(); - writer.WriteEndObject(); - writer.WriteEndObject(); - writer.WriteEndArray(); - }), - "tools/call" => BuildResponse(id, writer => + Name = "demo", + Version = "1.0.0" + }; + }) + .WithHttpTransport(options => { options.Stateless = true; }) + .WithTools(); + var app = builder.Build(); + app.Use(async (context, next) => + { + if (receivedHeaders.Count == 0 && + context.Request.Path.StartsWithSegments("/mcp", StringComparison.Ordinal)) + { + foreach (var header in context.Request.Headers) { - tracker.CallCalls++; - var text = document.RootElement.GetProperty("params").GetProperty("arguments").GetProperty("text").GetString(); - writer.WriteStartArray("content"); - writer.WriteStartObject(); - writer.WriteString("type", "text"); - writer.WriteString("text", $"demo:{text}"); - writer.WriteEndObject(); - writer.WriteEndArray(); - writer.WriteBoolean("isError", false); - }), - _ => throw new InvalidOperationException($"Unexpected MCP method '{method}'.") - }; - - context.Response.ContentType = "application/json"; - await context.Response.WriteAsync(response, context.RequestAborted); + receivedHeaders[header.Key] = header.Value.ToString(); + } + } + await TrackMcpMethodAsync(context, tracker); + await next(); }); + app.MapMcp("/mcp"); await app.StartAsync(); _apps.Add(app); var address = app.Urls.Single(); - return ($"{address.TrimEnd('/')}/", tracker); + return ($"{address.TrimEnd('/')}/mcp", tracker, receivedHeaders); } - private static string BuildResponse(string id, Action writeResult) + private static async Task TrackMcpMethodAsync(HttpContext context, McpCallTracker tracker) { - using var ms = new MemoryStream(); - using (var writer = new Utf8JsonWriter(ms)) + if (!context.Request.Path.StartsWithSegments("/mcp", StringComparison.Ordinal)) + return; + if (!HttpMethods.IsPost(context.Request.Method)) + return; + + context.Request.EnableBuffering(); + using var document = await JsonDocument.ParseAsync(context.Request.Body, cancellationToken: context.RequestAborted); + context.Request.Body.Position = 0; + + if (!document.RootElement.TryGetProperty("method", out var methodElement) || methodElement.ValueKind != JsonValueKind.String) + return; + var method = methodElement.GetString(); + switch (method) { - writer.WriteStartObject(); - writer.WriteString("jsonrpc", "2.0"); - writer.WriteString("id", id); - writer.WritePropertyName("result"); - writer.WriteStartObject(); - writeResult(writer); - writer.WriteEndObject(); - writer.WriteEndObject(); + case "initialize": + tracker.InitializeCalls++; + break; + case "tools/list": + tracker.ListCalls++; + break; + case "tools/call": + tracker.CallCalls++; + break; } - - return Encoding.UTF8.GetString(ms.ToArray()); } private sealed class McpCallTracker @@ -154,4 +207,12 @@ private sealed class McpCallTracker public int ListCalls { get; set; } public int CallCalls { get; set; } } + + [McpServerToolType] + private sealed class DemoMcpTools + { + [McpServerTool(Name = "echo", ReadOnly = true), Description("Demo echo tool")] + public string Echo([Description("text")] string text) + => $"demo:{text}"; + } } From 37428d806ae65261157a0992481b14d9118643ab Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 11:19:23 +0800 Subject: [PATCH 04/17] Extract McpNativeTool and improve registry Move the MCP native tool implementation into its own OpenClaw.Agent.Tools.McpNativeTool class and clean up McpServerToolRegistry logic. Ensure nativeRegistry.RegisterOwnedResource is called only once using Interlocked.Exchange. Refactor loading to accumulate discovered clients/tools before committing, properly dispose clients on failure (supporting IAsyncDisposable), and mark _loaded appropriately so failed attempts can be retried. Pass the correct cancellation token for tool listing and add tests to validate single owned-resource registration, retry behavior after failure, and request-timeout handling for tool listing. --- .../Plugins/McpServerToolRegistry.cs | 124 ++++++------------ src/OpenClaw.Agent/Tools/McpNativeTool.cs | 70 ++++++++++ .../McpServerToolRegistryTests.cs | 106 ++++++++++++++- 3 files changed, 210 insertions(+), 90 deletions(-) create mode 100644 src/OpenClaw.Agent/Tools/McpNativeTool.cs diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 912a3d1..203bda9 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -1,11 +1,10 @@ using System.Diagnostics; using System.Net.Http.Headers; using System.Text; -using System.Text.Json; using Microsoft.Extensions.Logging; using ModelContextProtocol; using ModelContextProtocol.Client; -using ModelContextProtocol.Protocol; +using OpenClaw.Agent.Tools; using OpenClaw.Core.Abstractions; using OpenClaw.Core.Plugins; using OpenClaw.Core.Security; @@ -23,6 +22,7 @@ public sealed class McpServerToolRegistry : IDisposable private readonly List _tools = []; private readonly List _clients = []; private bool _loaded; + private int _registeredAsOwnedResource; /// /// Creates a registry for configured MCP servers. @@ -39,7 +39,8 @@ public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) { var tools = await LoadAsync(ct); - nativeRegistry.RegisterOwnedResource(this); + if (Interlocked.Exchange(ref _registeredAsOwnedResource, 1) == 0) + nativeRegistry.RegisterOwnedResource(this); foreach (var tool in tools) nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); } @@ -49,46 +50,62 @@ internal async Task> LoadAsync(CancellationToke if (_loaded) return _tools; - _loaded = true; if (!_config.Enabled) + { + _loaded = true; return _tools; + } - foreach (var (serverId, serverConfig) in _config.Servers) - { - if (!serverConfig.Enabled) - continue; + var discoveredTools = new List(); + var discoveredClients = new List(); - var transport = CreateTransport(serverId, serverConfig); - McpClient? client = null; - try + try + { + foreach (var (serverId, serverConfig) in _config.Servers) { + if (!serverConfig.Enabled) + continue; + + var transport = CreateTransport(serverId, serverConfig); using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); - client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); - _clients.Add(client); + var client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); + discoveredClients.Add(client); var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; var pluginId = $"mcp:{serverId}"; - var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, timeoutCts.Token); + var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, ct); foreach (var tool in tools) { - _tools.Add(new DiscoveredMcpTool( + discoveredTools.Add(new DiscoveredMcpTool( pluginId, new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), displayName)); } } - catch + + _clients.AddRange(discoveredClients); + _tools.AddRange(discoveredTools); + _loaded = true; + return _tools; + } + catch + { + foreach (var client in discoveredClients) { - if (client is IDisposable disposable) - disposable.Dispose(); - throw; + try + { + (client as IDisposable)?.Dispose(); + (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + } + catch + { + } } + throw; } - - return _tools; } private async Task> LoadToolsFromClientAsync( @@ -237,70 +254,5 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig } internal sealed record DiscoveredMcpTool(string PluginId, ITool Tool, string Detail); - - private sealed class McpNativeTool( - McpClient client, - string localName, - string remoteName, - string description, - string parameterSchema) : ITool - { - public string Name => localName; - public string Description => description; - public string ParameterSchema => parameterSchema; - - public async ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) - { - try - { - using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); - var argsDict = new Dictionary(StringComparer.Ordinal); - foreach (var prop in argsDoc.RootElement.EnumerateObject()) - { - object? value = null; - var v = prop.Value; - switch (v.ValueKind) - { - case JsonValueKind.String: - value = v.GetString(); - break; - case JsonValueKind.Number: - value = v.TryGetInt64(out var l) ? l : v.GetDouble(); - break; - case JsonValueKind.True: - case JsonValueKind.False: - value = v.GetBoolean(); - break; - case JsonValueKind.Null: - value = null; - break; - default: - value = v.Clone(); - break; - } - argsDict[prop.Name] = value; - } - var response = await client.CallToolAsync(remoteName, argsDict, progress: null, cancellationToken: ct); - var parts = new List(); - foreach (var item in response.Content) - { - if (item is TextContentBlock t) - parts.Add(t.Text ?? ""); - } - var text = string.Join("\n\n", parts); - var isError = response.IsError ?? false; - return isError ? $"Error: {text}" : text; - } - catch (JsonException ex) - { - return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; - } - catch (Exception ex) - { - return $"Error: MCP tool '{localName}' failed: {ex.Message}"; - } - } - } - private sealed record McpToolDescriptor(string LocalName, string RemoteName, string Description, string InputSchemaText); } diff --git a/src/OpenClaw.Agent/Tools/McpNativeTool.cs b/src/OpenClaw.Agent/Tools/McpNativeTool.cs new file mode 100644 index 0000000..816dde8 --- /dev/null +++ b/src/OpenClaw.Agent/Tools/McpNativeTool.cs @@ -0,0 +1,70 @@ +using System.Text.Json; +using ModelContextProtocol.Client; +using ModelContextProtocol.Protocol; +using OpenClaw.Core.Abstractions; + +namespace OpenClaw.Agent.Tools; + +public sealed class McpNativeTool( + McpClient client, + string localName, + string remoteName, + string description, + string parameterSchema) : ITool +{ + public string Name => localName; + public string Description => description; + public string ParameterSchema => parameterSchema; + + public async ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) + { + try + { + using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); + var argsDict = new Dictionary(StringComparer.Ordinal); + foreach (var prop in argsDoc.RootElement.EnumerateObject()) + { + object? value = null; + var v = prop.Value; + switch (v.ValueKind) + { + case JsonValueKind.String: + value = v.GetString(); + break; + case JsonValueKind.Number: + value = v.TryGetInt64(out var l) ? l : v.GetDouble(); + break; + case JsonValueKind.True: + case JsonValueKind.False: + value = v.GetBoolean(); + break; + case JsonValueKind.Null: + value = null; + break; + default: + value = v.Clone(); + break; + } + argsDict[prop.Name] = value; + } + var response = await client.CallToolAsync(remoteName, argsDict, progress: null, cancellationToken: ct); + var parts = new List(); + foreach (var item in response.Content) + { + if (item is TextContentBlock t) + parts.Add(t.Text ?? ""); + } + var text = string.Join("\n\n", parts); + var isError = response.IsError ?? false; + return isError ? $"Error: {text}" : text; + } + catch (JsonException ex) + { + return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; + } + catch (Exception ex) + { + return $"Error: MCP tool '{localName}' failed: {ex.Message}"; + } + } +} diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index b963762..b955f42 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -98,13 +98,109 @@ public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() } } + [Fact] + public async Task RegisterToolsAsync_MultipleCalls_RegistersOwnedResourceOnlyOnce() + { + var (serverUrl, _) = await StartMcpServerAsync(); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + Assert.Single(nativeRegistry.Tools); + + var ownedResourcesField = typeof(NativePluginRegistry).GetField("_ownedResources", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + var ownedResources = Assert.IsType>(ownedResourcesField?.GetValue(nativeRegistry)); + Assert.Single(ownedResources); + } + + [Fact] + public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() + { + var (serverUrl, _) = await StartMcpServerAsync(); + var config = new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["broken"] = new() + { + Transport = "invalid-transport" + }, + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }; + var registry = new McpServerToolRegistry(config, NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await Assert.ThrowsAsync(() => registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None)); + var clientsField = typeof(McpServerToolRegistry).GetField("_clients", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + var clientsAfterFailure = Assert.IsType>(clientsField?.GetValue(registry)); + Assert.Empty(clientsAfterFailure); + + config.Servers["broken"].Enabled = false; + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + var clientsAfterSuccess = Assert.IsType>(clientsField?.GetValue(registry)); + Assert.Single(clientsAfterSuccess); + + var tool = Assert.Single(nativeRegistry.Tools); + Assert.Equal("demo.echo", tool.Name); + } + + [Fact] + public async Task LoadAsync_UsesRequestTimeoutForToolListing_NotStartupTimeout() + { + var (serverUrl, _) = await StartMcpServerAsync(TimeSpan.FromSeconds(2)); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl, + StartupTimeoutSeconds = 1, + RequestTimeoutSeconds = 5 + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + var tool = Assert.Single(nativeRegistry.Tools); + Assert.Equal("demo.echo", tool.Name); + } + public async ValueTask DisposeAsync() { foreach (var app in _apps) await app.DisposeAsync(); } - private async Task<(string ServerUrl, McpCallTracker Tracker)> StartMcpServerAsync() + private async Task<(string ServerUrl, McpCallTracker Tracker)> StartMcpServerAsync(TimeSpan? toolsListDelay = null) { var tracker = new McpCallTracker(); var builder = WebApplication.CreateSlimBuilder(); @@ -123,7 +219,7 @@ public async ValueTask DisposeAsync() var app = builder.Build(); app.Use(async (context, next) => { - await TrackMcpMethodAsync(context, tracker); + await TrackMcpMethodAsync(context, tracker, toolsListDelay); await next(); }); app.MapMcp("/mcp"); @@ -162,7 +258,7 @@ public async ValueTask DisposeAsync() receivedHeaders[header.Key] = header.Value.ToString(); } } - await TrackMcpMethodAsync(context, tracker); + await TrackMcpMethodAsync(context, tracker, null); await next(); }); app.MapMcp("/mcp"); @@ -173,7 +269,7 @@ public async ValueTask DisposeAsync() return ($"{address.TrimEnd('/')}/mcp", tracker, receivedHeaders); } - private static async Task TrackMcpMethodAsync(HttpContext context, McpCallTracker tracker) + private static async Task TrackMcpMethodAsync(HttpContext context, McpCallTracker tracker, TimeSpan? toolsListDelay) { if (!context.Request.Path.StartsWithSegments("/mcp", StringComparison.Ordinal)) return; @@ -193,6 +289,8 @@ private static async Task TrackMcpMethodAsync(HttpContext context, McpCallTracke tracker.InitializeCalls++; break; case "tools/list": + if (toolsListDelay is { } delay && delay > TimeSpan.Zero) + await Task.Delay(delay, context.RequestAborted); tracker.ListCalls++; break; case "tools/call": From ec4e9c33bfcc55b1b1f45b1b38a1efb270d6fefa Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 11:32:32 +0800 Subject: [PATCH 05/17] Skip validation for disabled MCP servers Ignore MCP servers that are explicitly disabled during config validation to avoid reporting missing required fields for disabled entries. Adds a guard in ConfigValidator to continue when server.Enabled is false, and a unit test (Validate_DisabledMcpServerWithMissingRequiredFields_DoesNotReturnError) to assert no validation errors are produced for disabled stdio/http servers with empty command/url. --- .../Validation/ConfigValidator.cs | 3 ++ src/OpenClaw.Tests/ConfigValidatorTests.cs | 34 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/src/OpenClaw.Core/Validation/ConfigValidator.cs b/src/OpenClaw.Core/Validation/ConfigValidator.cs index c009e07..df79c84 100644 --- a/src/OpenClaw.Core/Validation/ConfigValidator.cs +++ b/src/OpenClaw.Core/Validation/ConfigValidator.cs @@ -171,6 +171,9 @@ public static IReadOnlyList Validate(Models.GatewayConfig config) { foreach (var (serverId, server) in config.Plugins.Mcp.Servers) { + if (!server.Enabled) + continue; + var transport = NormalizeMcpTransport(server); if (transport is not ("stdio" or "http")) { diff --git a/src/OpenClaw.Tests/ConfigValidatorTests.cs b/src/OpenClaw.Tests/ConfigValidatorTests.cs index c834d6c..8ef2ec0 100644 --- a/src/OpenClaw.Tests/ConfigValidatorTests.cs +++ b/src/OpenClaw.Tests/ConfigValidatorTests.cs @@ -271,6 +271,40 @@ public void Validate_McpStdioServerWithoutCommand_ReturnsError() Assert.Contains(errors, e => e.Contains("Plugins.Mcp.Servers.demo.Command", StringComparison.Ordinal)); } + [Fact] + public void Validate_DisabledMcpServerWithMissingRequiredFields_DoesNotReturnError() + { + var config = new GatewayConfig + { + Plugins = new PluginsConfig + { + Mcp = new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["stdio-disabled"] = new() + { + Enabled = false, + Transport = "stdio", + Command = "" + }, + ["http-disabled"] = new() + { + Enabled = false, + Transport = "http", + Url = "" + } + } + } + } + }; + + var errors = ConfigValidator.Validate(config); + Assert.DoesNotContain(errors, e => e.Contains("Plugins.Mcp.Servers.stdio-disabled", StringComparison.Ordinal)); + Assert.DoesNotContain(errors, e => e.Contains("Plugins.Mcp.Servers.http-disabled", StringComparison.Ordinal)); + } + [Fact] public void Validate_OpenSandboxProviderWithoutEndpoint_ReturnsError() { From 5b9fb8e35f25c6817d406f0ddf6ec2c33d5c6537 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 11:37:23 +0800 Subject: [PATCH 06/17] Dispose displaced tools on name collision When registering an external tool with a duplicate name, previously the old registrations were removed but not disposed. Capture displaced tools before removing them and dispose any that implement IDisposable (and aren't the same instance) to avoid resource leaks. Add a unit test (RegisterExternalTool_NameCollision_DisposesDisplacedDisposableTool) and a DisposableFakeTool helper to verify the behavior. --- .../Plugins/NativePluginRegistry.cs | 6 ++++ src/OpenClaw.Tests/NativePluginTests.cs | 28 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs index 09b5e0d..c44f707 100644 --- a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs @@ -69,7 +69,13 @@ private void RegisterTool(ITool tool, string pluginId, string? detail = null) if (_nativeToolIds.ContainsKey(tool.Name)) { _logger.LogWarning("Duplicate native tool name '{ToolName}' from plugin '{PluginId}' — overwriting previous registration", tool.Name, pluginId); + var displacedTools = _tools.Where(t => t.Name == tool.Name).ToArray(); _tools.RemoveAll(t => t.Name == tool.Name); + foreach (var displaced in displacedTools) + { + if (!ReferenceEquals(displaced, tool) && displaced is IDisposable disposable) + disposable.Dispose(); + } } _tools.Add(tool); diff --git a/src/OpenClaw.Tests/NativePluginTests.cs b/src/OpenClaw.Tests/NativePluginTests.cs index 7803e9c..f286f1a 100644 --- a/src/OpenClaw.Tests/NativePluginTests.cs +++ b/src/OpenClaw.Tests/NativePluginTests.cs @@ -116,6 +116,22 @@ public void Dispose_DoesNotThrow() var registry = new NativePluginRegistry(config, NullLogger.Instance); registry.Dispose(); // should not throw } + + [Fact] + public void RegisterExternalTool_NameCollision_DisposesDisplacedDisposableTool() + { + using var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + var first = new DisposableFakeTool("dup_tool"); + var second = new DisposableFakeTool("dup_tool"); + + registry.RegisterExternalTool(first, "mcp:first"); + registry.RegisterExternalTool(second, "mcp:second"); + + Assert.Equal(1, first.DisposeCalls); + Assert.Equal(0, second.DisposeCalls); + Assert.Single(registry.Tools); + Assert.Same(second, registry.Tools[0]); + } } public class PluginPreferenceTests @@ -771,6 +787,18 @@ public ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct => ValueTask.FromResult("ok"); } +file sealed class DisposableFakeTool(string name) : ITool, IDisposable +{ + public int DisposeCalls { get; private set; } + public string Name => name; + public string Description => "disposable-fake"; + public string ParameterSchema => "{}"; + public ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) + => ValueTask.FromResult("ok"); + public void Dispose() + => DisposeCalls++; +} + /// Minimal ILogger for tests. file sealed class NullLogger : Microsoft.Extensions.Logging.ILogger { From 551bf315337b049639d6cc69a744ec58cff6ca8e Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 12:33:09 +0800 Subject: [PATCH 07/17] Use raw JSON schema and remove owned-resource Stop treating the registry as an owned resource and preserve MCP tool JSON schemas as raw JSON text. - Removed _ownedResources tracking, registration as an owned resource, and disposal of those resources from McpServerToolRegistry. - Added System.Text.Json usage and ResolveInputSchemaText(JsonElement) to return '{}' for null/undefined or the raw JSON text otherwise, so tool parameter schemas are emitted as proper JSON. - Updated tests to parse and assert the JSON schema structure and to expect the registry is not registered as an owned resource anymore (test renamed accordingly). These changes ensure schema fidelity when exposing remote MCP tool schemas and simplify ownership semantics for the registry. --- .../Plugins/McpServerToolRegistry.cs | 17 ++++++++++------- .../McpServerToolRegistryTests.cs | 12 ++++++++++-- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 203bda9..69549db 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -1,6 +1,7 @@ using System.Diagnostics; using System.Net.Http.Headers; using System.Text; +using System.Text.Json; using Microsoft.Extensions.Logging; using ModelContextProtocol; using ModelContextProtocol.Client; @@ -18,11 +19,9 @@ public sealed class McpServerToolRegistry : IDisposable { private readonly McpPluginsConfig _config; private readonly ILogger _logger; - private readonly List _ownedResources = []; private readonly List _tools = []; private readonly List _clients = []; private bool _loaded; - private int _registeredAsOwnedResource; /// /// Creates a registry for configured MCP servers. @@ -39,8 +38,6 @@ public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) { var tools = await LoadAsync(ct); - if (Interlocked.Exchange(ref _registeredAsOwnedResource, 1) == 0) - nativeRegistry.RegisterOwnedResource(this); foreach (var tool in tools) nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); } @@ -131,7 +128,7 @@ private async Task> LoadToolsFromClientAsync( var description = !string.IsNullOrWhiteSpace(tool.Description) ? $"{tool.Description} (from MCP server '{displayName}')" : $"MCP tool '{remoteName}' from server '{displayName}'."; - var inputSchema = "{}"; + var inputSchema = ResolveInputSchemaText(tool.JsonSchema); tools.Add(new McpToolDescriptor(localName, remoteName, description, inputSchema)); } @@ -166,10 +163,16 @@ private static string SanitizePrefixPart(string value) return sb.Length == 0 ? "mcp" : sb.ToString(); } + private static string ResolveInputSchemaText(JsonElement inputSchema) + { + if (inputSchema.ValueKind is JsonValueKind.Undefined or JsonValueKind.Null) + return "{}"; + + return inputSchema.GetRawText(); + } + public void Dispose() { - foreach (var resource in _ownedResources) - resource.Dispose(); foreach (var client in _clients) { try diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index b955f42..4ab8ae0 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -46,6 +46,14 @@ public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() var tool = Assert.Single(nativeRegistry.Tools); Assert.Equal("demo.echo", tool.Name); Assert.Contains("Demo echo tool", tool.Description, StringComparison.Ordinal); + using (var schemaDocument = JsonDocument.Parse(tool.ParameterSchema)) + { + var schemaRoot = schemaDocument.RootElement; + Assert.Equal(JsonValueKind.Object, schemaRoot.ValueKind); + Assert.True(schemaRoot.TryGetProperty("properties", out var properties)); + Assert.True(properties.TryGetProperty("text", out var textProperty)); + Assert.Equal(JsonValueKind.Object, textProperty.ValueKind); + } Assert.Equal("demo:hello", await tool.ExecuteAsync("""{"text":"hello"}""", CancellationToken.None)); Assert.True(calls.InitializeCalls >= 1); Assert.True(calls.ListCalls >= 1); @@ -99,7 +107,7 @@ public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() } [Fact] - public async Task RegisterToolsAsync_MultipleCalls_RegistersOwnedResourceOnlyOnce() + public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedResource() { var (serverUrl, _) = await StartMcpServerAsync(); var registry = new McpServerToolRegistry( @@ -125,7 +133,7 @@ public async Task RegisterToolsAsync_MultipleCalls_RegistersOwnedResourceOnlyOnc var ownedResourcesField = typeof(NativePluginRegistry).GetField("_ownedResources", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); var ownedResources = Assert.IsType>(ownedResourcesField?.GetValue(nativeRegistry)); - Assert.Single(ownedResources); + Assert.Empty(ownedResources); } [Fact] From 1d2144aa10cf32778fc3f99beccb7cd8cdb00fcb Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 12:40:05 +0800 Subject: [PATCH 08/17] Ignore disposal exceptions when replacing tools Wrap disposal of displaced native tools in a try/catch and log a warning if Dispose throws, so a failing Dispose won't abort plugin/tool registration. Adds a unit test (RegisterExternalTool_NameCollision_DisposeFailureDoesNotAbortRegistration) and a ThrowingDisposableFakeTool to verify that registration proceeds and the exception is logged when Dispose fails. --- .../Plugins/NativePluginRegistry.cs | 14 ++++++++- src/OpenClaw.Tests/NativePluginTests.cs | 31 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs index c44f707..c05b671 100644 --- a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs @@ -74,7 +74,19 @@ private void RegisterTool(ITool tool, string pluginId, string? detail = null) foreach (var displaced in displacedTools) { if (!ReferenceEquals(displaced, tool) && displaced is IDisposable disposable) - disposable.Dispose(); + { + try + { + disposable.Dispose(); + } + catch (Exception ex) + { + _logger.LogWarning(ex, + "Failed to dispose displaced native tool '{ToolName}' while registering plugin '{PluginId}'", + tool.Name, + pluginId); + } + } } } diff --git a/src/OpenClaw.Tests/NativePluginTests.cs b/src/OpenClaw.Tests/NativePluginTests.cs index f286f1a..d4b9dbd 100644 --- a/src/OpenClaw.Tests/NativePluginTests.cs +++ b/src/OpenClaw.Tests/NativePluginTests.cs @@ -132,6 +132,22 @@ public void RegisterExternalTool_NameCollision_DisposesDisplacedDisposableTool() Assert.Single(registry.Tools); Assert.Same(second, registry.Tools[0]); } + + [Fact] + public void RegisterExternalTool_NameCollision_DisposeFailureDoesNotAbortRegistration() + { + using var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + var first = new ThrowingDisposableFakeTool("dup_tool"); + var second = new DisposableFakeTool("dup_tool"); + + registry.RegisterExternalTool(first, "mcp:first"); + var ex = Record.Exception(() => registry.RegisterExternalTool(second, "mcp:second")); + + Assert.Null(ex); + Assert.Equal(1, first.DisposeCalls); + Assert.Single(registry.Tools); + Assert.Same(second, registry.Tools[0]); + } } public class PluginPreferenceTests @@ -799,6 +815,21 @@ public void Dispose() => DisposeCalls++; } +file sealed class ThrowingDisposableFakeTool(string name) : ITool, IDisposable +{ + public int DisposeCalls { get; private set; } + public string Name => name; + public string Description => "throwing-disposable-fake"; + public string ParameterSchema => "{}"; + public ValueTask ExecuteAsync(string argumentsJson, CancellationToken ct) + => ValueTask.FromResult("ok"); + public void Dispose() + { + DisposeCalls++; + throw new InvalidOperationException("dispose failed"); + } +} + /// Minimal ILogger for tests. file sealed class NullLogger : Microsoft.Extensions.Logging.ILogger { From 4d4ebd5a68617190c8cf353604d65037eeddca61 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 13:00:10 +0800 Subject: [PATCH 09/17] Guard LoadAsync with SemaphoreSlim Add a private SemaphoreSlim to serialize LoadAsync to prevent concurrent initialization. LoadAsync now waits on the semaphore, checks _loaded under the lock, and releases the semaphore in a finally block; client discovery and error cleanup are preserved. Dispose waits on the semaphore to avoid races with an in-flight load, disposes clients, then releases and disposes the semaphore. Add a unit test (LoadAsync_ConcurrentCalls_LoadsToolsOnce) to verify concurrent calls only perform a single discovery. --- .../Plugins/McpServerToolRegistry.cs | 124 ++++++++++-------- .../McpServerToolRegistryTests.cs | 29 ++++ 2 files changed, 100 insertions(+), 53 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 69549db..14bb1c7 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -19,6 +19,7 @@ public sealed class McpServerToolRegistry : IDisposable { private readonly McpPluginsConfig _config; private readonly ILogger _logger; + private readonly SemaphoreSlim _loadSemaphore = new(1, 1); private readonly List _tools = []; private readonly List _clients = []; private bool _loaded; @@ -44,64 +45,72 @@ public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, Cancel internal async Task> LoadAsync(CancellationToken ct) { - if (_loaded) - return _tools; - - if (!_config.Enabled) - { - _loaded = true; - return _tools; - } - - var discoveredTools = new List(); - var discoveredClients = new List(); - + await _loadSemaphore.WaitAsync(ct); try { - foreach (var (serverId, serverConfig) in _config.Servers) - { - if (!serverConfig.Enabled) - continue; - - var transport = CreateTransport(serverId, serverConfig); - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); - var client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); - discoveredClients.Add(client); + if (_loaded) + return _tools; - var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; - var pluginId = $"mcp:{serverId}"; + if (!_config.Enabled) + { + _loaded = true; + return _tools; + } - var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, ct); + var discoveredTools = new List(); + var discoveredClients = new List(); - foreach (var tool in tools) + try + { + foreach (var (serverId, serverConfig) in _config.Servers) { - discoveredTools.Add(new DiscoveredMcpTool( - pluginId, - new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), - displayName)); + if (!serverConfig.Enabled) + continue; + + var transport = CreateTransport(serverId, serverConfig); + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); + var client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); + discoveredClients.Add(client); + + var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; + var pluginId = $"mcp:{serverId}"; + + var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, ct); + + foreach (var tool in tools) + { + discoveredTools.Add(new DiscoveredMcpTool( + pluginId, + new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), + displayName)); + } } - } - _clients.AddRange(discoveredClients); - _tools.AddRange(discoveredTools); - _loaded = true; - return _tools; - } - catch - { - foreach (var client in discoveredClients) + _clients.AddRange(discoveredClients); + _tools.AddRange(discoveredTools); + _loaded = true; + return _tools; + } + catch { - try - { - (client as IDisposable)?.Dispose(); - (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); - } - catch + foreach (var client in discoveredClients) { + try + { + (client as IDisposable)?.Dispose(); + (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + } + catch + { + } } + throw; } - throw; + } + finally + { + _loadSemaphore.Release(); } } @@ -173,17 +182,26 @@ private static string ResolveInputSchemaText(JsonElement inputSchema) public void Dispose() { - foreach (var client in _clients) + _loadSemaphore.Wait(); + try { - try - { - (client as IDisposable)?.Dispose(); - (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); - } - catch + foreach (var client in _clients) { + try + { + (client as IDisposable)?.Dispose(); + (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + } + catch + { + } } } + finally + { + _loadSemaphore.Release(); + _loadSemaphore.Dispose(); + } } diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index 4ab8ae0..39f2bcf 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -136,6 +136,35 @@ public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedRes Assert.Empty(ownedResources); } + [Fact] + public async Task LoadAsync_ConcurrentCalls_LoadsToolsOnce() + { + var (serverUrl, calls) = await StartMcpServerAsync(); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + + var loads = await Task.WhenAll( + registry.LoadAsync(CancellationToken.None), + registry.LoadAsync(CancellationToken.None), + registry.LoadAsync(CancellationToken.None), + registry.LoadAsync(CancellationToken.None)); + + Assert.All(loads, tools => Assert.Single(tools)); + Assert.Equal(1, calls.ListCalls); + } + [Fact] public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() { From 4ac7b2568fd0dea0b9876391bb447f6b68b72c68 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 13:25:36 +0800 Subject: [PATCH 10/17] Prevent duplicate owned resource registration Validate and deduplicate owned resources in NativePluginRegistry.RegisterOwnedResource: throw on null, ignore if the same instance is already registered, otherwise add. Adds tests covering null argument, duplicate registration resulting in a single dispose call, and a DisposableOwnedResource test helper. --- .../Plugins/NativePluginRegistry.cs | 7 ++++- src/OpenClaw.Tests/NativePluginTests.cs | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs index c05b671..676aaf4 100644 --- a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs @@ -100,7 +100,12 @@ public void RegisterExternalTool(ITool tool, string pluginId, string? detail = n => RegisterTool(tool, pluginId, detail); public void RegisterOwnedResource(IDisposable resource) - => _ownedResources.Add(resource); + { + ArgumentNullException.ThrowIfNull(resource); + if (_ownedResources.Any(existing => ReferenceEquals(existing, resource))) + return; + _ownedResources.Add(resource); + } /// /// All enabled native plugin tools. diff --git a/src/OpenClaw.Tests/NativePluginTests.cs b/src/OpenClaw.Tests/NativePluginTests.cs index d4b9dbd..91b0d91 100644 --- a/src/OpenClaw.Tests/NativePluginTests.cs +++ b/src/OpenClaw.Tests/NativePluginTests.cs @@ -148,6 +148,26 @@ public void RegisterExternalTool_NameCollision_DisposeFailureDoesNotAbortRegistr Assert.Single(registry.Tools); Assert.Same(second, registry.Tools[0]); } + + [Fact] + public void RegisterOwnedResource_Null_ThrowsArgumentNullException() + { + using var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + Assert.Throws(() => registry.RegisterOwnedResource(null!)); + } + + [Fact] + public void RegisterOwnedResource_SameInstanceTwice_DisposesOnce() + { + var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + var resource = new DisposableOwnedResource(); + + registry.RegisterOwnedResource(resource); + registry.RegisterOwnedResource(resource); + registry.Dispose(); + + Assert.Equal(1, resource.DisposeCalls); + } } public class PluginPreferenceTests @@ -830,6 +850,13 @@ public void Dispose() } } +file sealed class DisposableOwnedResource : IDisposable +{ + public int DisposeCalls { get; private set; } + public void Dispose() + => DisposeCalls++; +} + /// Minimal ILogger for tests. file sealed class NullLogger : Microsoft.Extensions.Logging.ILogger { From 942db3d1ad1a498ed9651f784d9b52e1955c2157 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 13:35:33 +0800 Subject: [PATCH 11/17] Validate MCP tool JSON arguments are objects Add a check in McpNativeTool to ensure the parsed JSON root is an object and return a clear error message if not. This avoids exceptions from calling EnumerateObject() on non-object roots (arrays or primitives) when processing tool arguments. --- src/OpenClaw.Agent/Tools/McpNativeTool.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/OpenClaw.Agent/Tools/McpNativeTool.cs b/src/OpenClaw.Agent/Tools/McpNativeTool.cs index 816dde8..7f67dbc 100644 --- a/src/OpenClaw.Agent/Tools/McpNativeTool.cs +++ b/src/OpenClaw.Agent/Tools/McpNativeTool.cs @@ -21,6 +21,8 @@ public async ValueTask ExecuteAsync(string argumentsJson, CancellationTo try { using var argsDoc = JsonDocument.Parse(string.IsNullOrWhiteSpace(argumentsJson) ? "{}" : argumentsJson); + if (argsDoc.RootElement.ValueKind != JsonValueKind.Object) + return $"Error: Invalid JSON arguments for MCP tool '{localName}': JSON root must be an object."; var argsDict = new Dictionary(StringComparer.Ordinal); foreach (var prop in argsDoc.RootElement.EnumerateObject()) { From 80b8f7934bf5af9fe4b63663c75525d571acf94e Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 14:21:44 +0800 Subject: [PATCH 12/17] Refactor MCP transport and fix lifecycle bugs Centralize MCP transport normalization into McpServerConfig.NormalizeTransport() and update callers (ConfigValidator and McpServerToolRegistry) to use the extension; remove duplicate NormalizeTransport implementation. Add a _registered guard to McpServerToolRegistry.RegisterToolsAsync to avoid double registration. Improve Dispose() to wait for the load semaphore with a 5s timeout, log a warning if acquiring fails, and only release the semaphore if it was acquired. Fix JSON number handling in McpNativeTool by cloning the JsonElement value to preserve numeric fidelity. Also add the required using for plugins namespace. --- .../Plugins/McpServerToolRegistry.cs | 33 +++++++++---------- src/OpenClaw.Agent/Tools/McpNativeTool.cs | 2 +- src/OpenClaw.Core/Plugins/PluginModels.cs | 18 ++++++++++ .../Validation/ConfigValidator.cs | 18 ++-------- 4 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 14bb1c7..776940a 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -23,6 +23,7 @@ public sealed class McpServerToolRegistry : IDisposable private readonly List _tools = []; private readonly List _clients = []; private bool _loaded; + private bool _registered; /// /// Creates a registry for configured MCP servers. @@ -38,9 +39,14 @@ public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) /// public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) { + if (_registered) + return; + var tools = await LoadAsync(ct); foreach (var tool in tools) nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); + + _registered = true; } internal async Task> LoadAsync(CancellationToken ct) @@ -182,9 +188,14 @@ private static string ResolveInputSchemaText(JsonElement inputSchema) public void Dispose() { - _loadSemaphore.Wait(); + bool acquired = false; try { + acquired = _loadSemaphore.Wait(TimeSpan.FromSeconds(5)); + if (!acquired) + { + _logger.LogWarning("McpServerToolRegistry.Dispose() timed out waiting for load semaphore"); + } foreach (var client in _clients) { try @@ -199,31 +210,17 @@ public void Dispose() } finally { - _loadSemaphore.Release(); + if (acquired) + _loadSemaphore.Release(); _loadSemaphore.Dispose(); } } - private static string NormalizeTransport(McpServerConfig config) - { - var transport = config.Transport?.Trim(); - if (string.IsNullOrWhiteSpace(transport)) - return string.IsNullOrWhiteSpace(config.Url) ? "stdio" : "http"; - - if (transport.Equals("streamable-http", StringComparison.OrdinalIgnoreCase) || - transport.Equals("streamable_http", StringComparison.OrdinalIgnoreCase)) - { - return "http"; - } - - return transport.ToLowerInvariant(); - } - private static IClientTransport CreateTransport(string serverId, McpServerConfig config) { - var transport = NormalizeTransport(config); + var transport = config.NormalizeTransport(); return transport switch { "stdio" => new StdioClientTransport(new StdioClientTransportOptions diff --git a/src/OpenClaw.Agent/Tools/McpNativeTool.cs b/src/OpenClaw.Agent/Tools/McpNativeTool.cs index 7f67dbc..e494aa4 100644 --- a/src/OpenClaw.Agent/Tools/McpNativeTool.cs +++ b/src/OpenClaw.Agent/Tools/McpNativeTool.cs @@ -34,7 +34,7 @@ public async ValueTask ExecuteAsync(string argumentsJson, CancellationTo value = v.GetString(); break; case JsonValueKind.Number: - value = v.TryGetInt64(out var l) ? l : v.GetDouble(); + value = v.Clone(); break; case JsonValueKind.True: case JsonValueKind.False: diff --git a/src/OpenClaw.Core/Plugins/PluginModels.cs b/src/OpenClaw.Core/Plugins/PluginModels.cs index f719f8a..d51d65c 100644 --- a/src/OpenClaw.Core/Plugins/PluginModels.cs +++ b/src/OpenClaw.Core/Plugins/PluginModels.cs @@ -135,6 +135,24 @@ public sealed class McpServerConfig public int RequestTimeoutSeconds { get; set; } = 60; } +public static class McpServerConfigExtensions +{ + public static string NormalizeTransport(this McpServerConfig config) + { + var transport = config.Transport?.Trim(); + if (string.IsNullOrWhiteSpace(transport)) + return string.IsNullOrWhiteSpace(config.Url) ? "stdio" : "http"; + + if (transport.Equals("streamable-http", StringComparison.OrdinalIgnoreCase) || + transport.Equals("streamable_http", StringComparison.OrdinalIgnoreCase)) + { + return "http"; + } + + return transport.ToLowerInvariant(); + } +} + /// /// Configuration for native (C#) replicas of popular OpenClaw plugins. /// Each property matches the canonical plugin id. diff --git a/src/OpenClaw.Core/Validation/ConfigValidator.cs b/src/OpenClaw.Core/Validation/ConfigValidator.cs index df79c84..0edb02b 100644 --- a/src/OpenClaw.Core/Validation/ConfigValidator.cs +++ b/src/OpenClaw.Core/Validation/ConfigValidator.cs @@ -1,5 +1,6 @@ using OpenClaw.Core.Security; using OpenClaw.Core.Models; +using OpenClaw.Core.Plugins; namespace OpenClaw.Core.Validation; @@ -174,7 +175,7 @@ public static IReadOnlyList Validate(Models.GatewayConfig config) if (!server.Enabled) continue; - var transport = NormalizeMcpTransport(server); + var transport = server.NormalizeTransport(); if (transport is not ("stdio" or "http")) { errors.Add($"Plugins.Mcp.Servers.{serverId}.Transport must be 'stdio' or 'http'."); @@ -409,19 +410,4 @@ private static void ValidateDmPolicy(string field, string? value, ICollection Date: Fri, 27 Mar 2026 15:01:17 +0800 Subject: [PATCH 13/17] Improve teardown and secret resolution in MCP registry McpServerToolRegistry: strengthen Dispose to wait indefinitely if the initial semaphore wait times out, handle ObjectDisposedException and log appropriately, and ensure load completion ordering. Secret resolution for environment variables and headers now treats unresolved values that start with "env:" as errors (throws InvalidOperationException) and preserves resolved/null semantics. Tests: update multiple tests to dispose the registry via "using var registry =" to match IDisposable behavior and adjust one test to construct the registry with a provided logger. These changes tighten lifecycle handling and make secret/env resolution failures explicit. --- .../Plugins/McpServerToolRegistry.cs | 25 +++++++++++++++---- .../McpServerToolRegistryTests.cs | 13 +++++----- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 776940a..5995db3 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -194,8 +194,19 @@ public void Dispose() acquired = _loadSemaphore.Wait(TimeSpan.FromSeconds(5)); if (!acquired) { - _logger.LogWarning("McpServerToolRegistry.Dispose() timed out waiting for load semaphore"); + _logger.LogWarning("McpServerToolRegistry.Dispose() timed out waiting for load semaphore, waiting indefinitely to ensure load completes"); + _loadSemaphore.Wait(); + acquired = true; } + } + catch (ObjectDisposedException) + { + _logger.LogWarning("McpServerToolRegistry.Dispose() encountered disposed semaphore, load may have completed concurrently"); + return; + } + + try + { foreach (var client in _clients) { try @@ -249,8 +260,10 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig var resolved = new Dictionary(StringComparer.Ordinal); foreach (var (name, rawValue) in environment) { - var value = SecretResolver.Resolve(rawValue) ?? rawValue; - resolved[name] = value; + var value = SecretResolver.Resolve(rawValue); + if (value is null && rawValue.StartsWith("env:", StringComparison.Ordinal)) + throw new InvalidOperationException($"Environment variable '{name}' references unset env var '{rawValue[4..]}'"); + resolved[name] = value ?? rawValue; } return resolved; @@ -264,8 +277,10 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig var resolved = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var (name, rawValue) in headers) { - var value = SecretResolver.Resolve(rawValue) ?? rawValue; - resolved[name] = value; + var value = SecretResolver.Resolve(rawValue); + if (value is null && rawValue.StartsWith("env:", StringComparison.Ordinal)) + throw new InvalidOperationException($"Header '{name}' references unset env var '{rawValue[4..]}'"); + resolved[name] = value ?? rawValue; } return resolved; diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index 39f2bcf..52b7730 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -25,7 +25,7 @@ public sealed class McpServerToolRegistryTests : IAsyncDisposable public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() { var (serverUrl, calls) = await StartMcpServerAsync(); - var registry = new McpServerToolRegistry( + using var registry = new McpServerToolRegistry( new McpPluginsConfig { Enabled = true, @@ -63,12 +63,11 @@ public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() [Fact] public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() { - // Set up environment variable for testing Environment.SetEnvironmentVariable("TEST_AUTH_TOKEN", "secret-token-123"); try { var (serverUrl, calls, receivedHeaders) = await StartMcpServerWithHeaderCheckAsync(); - var registry = new McpServerToolRegistry( + using var registry = new McpServerToolRegistry( new McpPluginsConfig { Enabled = true, @@ -110,7 +109,7 @@ public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedResource() { var (serverUrl, _) = await StartMcpServerAsync(); - var registry = new McpServerToolRegistry( + using var registry = new McpServerToolRegistry( new McpPluginsConfig { Enabled = true, @@ -140,7 +139,7 @@ public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedRes public async Task LoadAsync_ConcurrentCalls_LoadsToolsOnce() { var (serverUrl, calls) = await StartMcpServerAsync(); - var registry = new McpServerToolRegistry( + using var registry = new McpServerToolRegistry( new McpPluginsConfig { Enabled = true, @@ -185,7 +184,7 @@ public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() } } }; - var registry = new McpServerToolRegistry(config, NullLogger.Instance); + using var registry = new McpServerToolRegistry(config, NullLogger.Instance); using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); await Assert.ThrowsAsync(() => registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None)); @@ -207,7 +206,7 @@ public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() public async Task LoadAsync_UsesRequestTimeoutForToolListing_NotStartupTimeout() { var (serverUrl, _) = await StartMcpServerAsync(TimeSpan.FromSeconds(2)); - var registry = new McpServerToolRegistry( + using var registry = new McpServerToolRegistry( new McpPluginsConfig { Enabled = true, From 535d646beffcaa305f9499f892a8728a02e46e96 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 15:32:24 +0800 Subject: [PATCH 14/17] Improve MCP tool registry concurrency and safety Add proper semaphore locking around tool loading and registration to prevent races, extract LoadInternalAsync to simplify load logic, and ensure clients are disposed on failure. Only register MCP tools at startup when Plugins.Mcp is enabled, and include Plugins.Mcp in the security check that forbids third-party plugins on public binds. Also remove unused usings and tidy up client/tool collection handling. --- .../Plugins/McpServerToolRegistry.cs | 131 ++++++++++-------- .../RuntimeInitializationExtensions.cs | 5 +- .../Extensions/GatewaySecurityExtensions.cs | 2 +- 3 files changed, 76 insertions(+), 62 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index 5995db3..fe41983 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -1,5 +1,3 @@ -using System.Diagnostics; -using System.Net.Http.Headers; using System.Text; using System.Text.Json; using Microsoft.Extensions.Logging; @@ -39,14 +37,22 @@ public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) /// public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) { - if (_registered) - return; + await _loadSemaphore.WaitAsync(ct); + try + { + if (_registered) + return; - var tools = await LoadAsync(ct); - foreach (var tool in tools) - nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); + var tools = await LoadInternalAsync(ct); + foreach (var tool in tools) + nativeRegistry.RegisterExternalTool(tool.Tool, tool.PluginId, tool.Detail); - _registered = true; + _registered = true; + } + finally + { + _loadSemaphore.Release(); + } } internal async Task> LoadAsync(CancellationToken ct) @@ -54,69 +60,74 @@ internal async Task> LoadAsync(CancellationToke await _loadSemaphore.WaitAsync(ct); try { - if (_loaded) - return _tools; + return _loaded ? _tools : await LoadInternalAsync(ct); + } + finally + { + _loadSemaphore.Release(); + } + } - if (!_config.Enabled) - { - _loaded = true; - return _tools; - } + private async Task> LoadInternalAsync(CancellationToken ct) + { + if (_loaded) + return _tools; + + if (!_config.Enabled) + { + _loaded = true; + return _tools; + } - var discoveredTools = new List(); - var discoveredClients = new List(); + var discoveredTools = new List(); + var discoveredClients = new List(); - try + try + { + foreach (var (serverId, serverConfig) in _config.Servers) { - foreach (var (serverId, serverConfig) in _config.Servers) + if (!serverConfig.Enabled) + continue; + + var transport = CreateTransport(serverId, serverConfig); + using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); + var client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); + discoveredClients.Add(client); + + var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; + var pluginId = $"mcp:{serverId}"; + + var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, ct); + + foreach (var tool in tools) { - if (!serverConfig.Enabled) - continue; - - var transport = CreateTransport(serverId, serverConfig); - using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - timeoutCts.CancelAfter(TimeSpan.FromSeconds(serverConfig.StartupTimeoutSeconds)); - var client = await McpClient.CreateAsync(transport, cancellationToken: timeoutCts.Token); - discoveredClients.Add(client); - - var displayName = string.IsNullOrWhiteSpace(serverConfig.Name) ? serverId : serverConfig.Name!; - var pluginId = $"mcp:{serverId}"; - - var tools = await LoadToolsFromClientAsync(client, serverId, pluginId, displayName, serverConfig, ct); - - foreach (var tool in tools) - { - discoveredTools.Add(new DiscoveredMcpTool( - pluginId, - new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), - displayName)); - } + discoveredTools.Add(new DiscoveredMcpTool( + pluginId, + new McpNativeTool(client, tool.LocalName, tool.RemoteName, tool.Description, tool.InputSchemaText), + displayName)); } - - _clients.AddRange(discoveredClients); - _tools.AddRange(discoveredTools); - _loaded = true; - return _tools; } - catch + + _clients.AddRange(discoveredClients); + _tools.AddRange(discoveredTools); + _loaded = true; + return _tools; + } + catch + { + foreach (var client in discoveredClients) { - foreach (var client in discoveredClients) + try + { + (client as IDisposable)?.Dispose(); + (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + } + catch { - try - { - (client as IDisposable)?.Dispose(); - (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); - } - catch - { - } } - throw; } - } - finally - { - _loadSemaphore.Release(); + throw; } } diff --git a/src/OpenClaw.Gateway/Composition/RuntimeInitializationExtensions.cs b/src/OpenClaw.Gateway/Composition/RuntimeInitializationExtensions.cs index fac5f5e..463ac9c 100644 --- a/src/OpenClaw.Gateway/Composition/RuntimeInitializationExtensions.cs +++ b/src/OpenClaw.Gateway/Composition/RuntimeInitializationExtensions.cs @@ -120,7 +120,10 @@ public static async Task InitializeOpenClawRuntimeAsync( loggerFactory.CreateLogger()); var builtInTools = CreateBuiltInTools(config, memoryStore, sessionManager, pipeline, startup.WorkspacePath); - await mcpRegistry.RegisterToolsAsync(nativeRegistry, app.Lifetime.ApplicationStopping); + if (config.Plugins.Mcp.Enabled) + { + await mcpRegistry.RegisterToolsAsync(nativeRegistry, app.Lifetime.ApplicationStopping); + } LlmClientFactory.ResetDynamicProviders(); try { diff --git a/src/OpenClaw.Gateway/Extensions/GatewaySecurityExtensions.cs b/src/OpenClaw.Gateway/Extensions/GatewaySecurityExtensions.cs index 2958d89..79190c9 100644 --- a/src/OpenClaw.Gateway/Extensions/GatewaySecurityExtensions.cs +++ b/src/OpenClaw.Gateway/Extensions/GatewaySecurityExtensions.cs @@ -28,7 +28,7 @@ public static void EnforcePublicBindHardening(GatewayConfig config, bool isNonLo "or explicitly opt in via OpenClaw:Security:AllowUnsafeToolingOnPublicBind=true."); } - if ((config.Plugins.Enabled || config.Plugins.DynamicNative.Enabled) && !config.Security.AllowPluginBridgeOnPublicBind) + if ((config.Plugins.Enabled || config.Plugins.DynamicNative.Enabled || config.Plugins.Mcp.Enabled) && !config.Security.AllowPluginBridgeOnPublicBind) { throw new InvalidOperationException( "Refusing to start with third-party plugin execution enabled on a non-loopback bind. " + From a22d4ed2c4eba76e8a28e44129b196c1dc0ca52a Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 16:02:28 +0800 Subject: [PATCH 15/17] Handle null collections and cancellation Avoid null-reference exceptions and improve cancellation handling: - Iterate safely over _config.Servers by defaulting to an empty collection when Servers may be null. - Make ResolveEnv and ResolveHeaders accept nullable Dictionary parameters and return null for null/empty inputs to reflect absence of values. - Add an explicit catch for OperationCanceledException that rethrows when the provided CancellationToken is cancelled so cancellation propagates correctly. These changes harden MCP tool registration and execution against missing configuration and ensure proper task cancellation semantics. --- src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs | 10 +++++----- src/OpenClaw.Agent/Tools/McpNativeTool.cs | 4 ++++ 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index fe41983..a7acacd 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -84,7 +84,7 @@ private async Task> LoadInternalAsync(Cancellat try { - foreach (var (serverId, serverConfig) in _config.Servers) + foreach (var (serverId, serverConfig) in _config.Servers ?? []) { if (!serverConfig.Enabled) continue; @@ -263,9 +263,9 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig }; } - private static Dictionary? ResolveEnv(Dictionary environment) + private static Dictionary? ResolveEnv(Dictionary? environment) { - if (environment.Count == 0) + if (environment is null || environment.Count == 0) return null; var resolved = new Dictionary(StringComparer.Ordinal); @@ -280,9 +280,9 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig return resolved; } - private static Dictionary? ResolveHeaders(Dictionary headers) + private static Dictionary? ResolveHeaders(Dictionary? headers) { - if (headers.Count == 0) + if (headers is null || headers.Count == 0) return null; var resolved = new Dictionary(StringComparer.OrdinalIgnoreCase); diff --git a/src/OpenClaw.Agent/Tools/McpNativeTool.cs b/src/OpenClaw.Agent/Tools/McpNativeTool.cs index e494aa4..ce1a006 100644 --- a/src/OpenClaw.Agent/Tools/McpNativeTool.cs +++ b/src/OpenClaw.Agent/Tools/McpNativeTool.cs @@ -64,6 +64,10 @@ public async ValueTask ExecuteAsync(string argumentsJson, CancellationTo { return $"Error: Invalid JSON arguments for MCP tool '{localName}': {ex.Message}"; } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; + } catch (Exception ex) { return $"Error: MCP tool '{localName}' failed: {ex.Message}"; From 4f4e88e45a6b813e56876065d02fd49331900097 Mon Sep 17 00:00:00 2001 From: geffzhang Date: Fri, 27 Mar 2026 16:31:51 +0800 Subject: [PATCH 16/17] Validate MCP servers and handle null config values Add null-safety and stronger validation for MCP plugin configuration. In McpServerToolRegistry: default stdio Arguments to an empty array, and handle null raw environment/header values (preserving null for env vars and using empty string for headers) to avoid null-ref errors during resolution. In ConfigValidator: return a validation error when Plugins.Mcp is enabled but Servers is null, and restructure the servers loop while keeping existing checks for transport type, command presence for stdio, URL validity for http, and startup/request timeouts. --- .../Plugins/McpServerToolRegistry.cs | 12 ++++- .../Validation/ConfigValidator.cs | 51 +++++++++++-------- 2 files changed, 40 insertions(+), 23 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index a7acacd..b43727b 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -248,7 +248,7 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig "stdio" => new StdioClientTransport(new StdioClientTransportOptions { Command = config.Command!, - Arguments = config.Arguments, + Arguments = config.Arguments ?? [], WorkingDirectory = config.WorkingDirectory, EnvironmentVariables = ResolveEnv(config.Environment), Name = serverId, @@ -271,6 +271,11 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig var resolved = new Dictionary(StringComparer.Ordinal); foreach (var (name, rawValue) in environment) { + if (rawValue is null) + { + resolved[name] = null; + continue; + } var value = SecretResolver.Resolve(rawValue); if (value is null && rawValue.StartsWith("env:", StringComparison.Ordinal)) throw new InvalidOperationException($"Environment variable '{name}' references unset env var '{rawValue[4..]}'"); @@ -288,6 +293,11 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig var resolved = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var (name, rawValue) in headers) { + if (rawValue is null) + { + resolved[name] = string.Empty; + continue; + } var value = SecretResolver.Resolve(rawValue); if (value is null && rawValue.StartsWith("env:", StringComparison.Ordinal)) throw new InvalidOperationException($"Header '{name}' references unset env var '{rawValue[4..]}'"); diff --git a/src/OpenClaw.Core/Validation/ConfigValidator.cs b/src/OpenClaw.Core/Validation/ConfigValidator.cs index 0edb02b..e290dfb 100644 --- a/src/OpenClaw.Core/Validation/ConfigValidator.cs +++ b/src/OpenClaw.Core/Validation/ConfigValidator.cs @@ -170,32 +170,39 @@ public static IReadOnlyList Validate(Models.GatewayConfig config) // MCP plugin servers if (config.Plugins.Mcp.Enabled) { - foreach (var (serverId, server) in config.Plugins.Mcp.Servers) + if (config.Plugins.Mcp.Servers is null) { - if (!server.Enabled) - continue; - - var transport = server.NormalizeTransport(); - if (transport is not ("stdio" or "http")) + errors.Add("Plugins.Mcp.Servers must be provided when MCP is enabled."); + } + else + { + foreach (var (serverId, server) in config.Plugins.Mcp.Servers) { - errors.Add($"Plugins.Mcp.Servers.{serverId}.Transport must be 'stdio' or 'http'."); - continue; - } + if (!server.Enabled) + continue; - if (server.StartupTimeoutSeconds < 1) - errors.Add($"Plugins.Mcp.Servers.{serverId}.StartupTimeoutSeconds must be >= 1 (got {server.StartupTimeoutSeconds})."); - if (server.RequestTimeoutSeconds < 1) - errors.Add($"Plugins.Mcp.Servers.{serverId}.RequestTimeoutSeconds must be >= 1 (got {server.RequestTimeoutSeconds})."); + var transport = server.NormalizeTransport(); + if (transport is not ("stdio" or "http")) + { + errors.Add($"Plugins.Mcp.Servers.{serverId}.Transport must be 'stdio' or 'http'."); + continue; + } - if (transport == "stdio") - { - if (string.IsNullOrWhiteSpace(server.Command)) - errors.Add($"Plugins.Mcp.Servers.{serverId}.Command must be set when Transport='stdio'."); - } - else if (!Uri.TryCreate(server.Url, UriKind.Absolute, out var url) || - (url.Scheme != Uri.UriSchemeHttp && url.Scheme != Uri.UriSchemeHttps)) - { - errors.Add($"Plugins.Mcp.Servers.{serverId}.Url must be an absolute http(s) URL when Transport='http'."); + if (server.StartupTimeoutSeconds < 1) + errors.Add($"Plugins.Mcp.Servers.{serverId}.StartupTimeoutSeconds must be >= 1 (got {server.StartupTimeoutSeconds})."); + if (server.RequestTimeoutSeconds < 1) + errors.Add($"Plugins.Mcp.Servers.{serverId}.RequestTimeoutSeconds must be >= 1 (got {server.RequestTimeoutSeconds})."); + + if (transport == "stdio") + { + if (string.IsNullOrWhiteSpace(server.Command)) + errors.Add($"Plugins.Mcp.Servers.{serverId}.Command must be set when Transport='stdio'."); + } + else if (!Uri.TryCreate(server.Url, UriKind.Absolute, out var url) || + (url.Scheme != Uri.UriSchemeHttp && url.Scheme != Uri.UriSchemeHttps)) + { + errors.Add($"Plugins.Mcp.Servers.{serverId}.Url must be an absolute http(s) URL when Transport='http'."); + } } } } From dcaac072c712771386f010f2bc27c2fe7cf0e1d7 Mon Sep 17 00:00:00 2001 From: telli Date: Fri, 27 Mar 2026 13:20:44 -0700 Subject: [PATCH 17/17] Fix MCP tool handling and registry shutdown --- .../Plugins/McpServerToolRegistry.cs | 37 ++++- .../Plugins/NativePluginRegistry.cs | 22 ++- src/OpenClaw.Agent/Tools/McpNativeTool.cs | 55 ++++++- .../McpServerToolRegistryTests.cs | 148 ++++++++++++++++-- src/OpenClaw.Tests/NativePluginTests.cs | 44 ++++++ 5 files changed, 280 insertions(+), 26 deletions(-) diff --git a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs index b43727b..c33cea2 100644 --- a/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/McpServerToolRegistry.cs @@ -22,6 +22,7 @@ public sealed class McpServerToolRegistry : IDisposable private readonly List _clients = []; private bool _loaded; private bool _registered; + private bool _disposed; /// /// Creates a registry for configured MCP servers. @@ -37,9 +38,11 @@ public McpServerToolRegistry(McpPluginsConfig config, ILogger logger) /// public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, CancellationToken ct) { + ThrowIfDisposed(); await _loadSemaphore.WaitAsync(ct); try { + ThrowIfDisposed(); if (_registered) return; @@ -57,9 +60,11 @@ public async Task RegisterToolsAsync(NativePluginRegistry nativeRegistry, Cancel internal async Task> LoadAsync(CancellationToken ct) { + ThrowIfDisposed(); await _loadSemaphore.WaitAsync(ct); try { + ThrowIfDisposed(); return _loaded ? _tools : await LoadInternalAsync(ct); } finally @@ -120,8 +125,7 @@ private async Task> LoadInternalAsync(Cancellat { try { - (client as IDisposable)?.Dispose(); - (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + DisposeClient(client); } catch { @@ -218,28 +222,29 @@ public void Dispose() try { + if (_disposed) + return; + + _disposed = true; foreach (var client in _clients) { try { - (client as IDisposable)?.Dispose(); - (client as IAsyncDisposable)?.DisposeAsync().GetAwaiter().GetResult(); + DisposeClient(client); } catch { } } + _clients.Clear(); } finally { if (acquired) _loadSemaphore.Release(); - _loadSemaphore.Dispose(); } } - - private static IClientTransport CreateTransport(string serverId, McpServerConfig config) { var transport = config.NormalizeTransport(); @@ -307,6 +312,24 @@ private static IClientTransport CreateTransport(string serverId, McpServerConfig return resolved; } + private void ThrowIfDisposed() + { + if (_disposed) + throw new ObjectDisposedException(nameof(McpServerToolRegistry)); + } + + private static void DisposeClient(McpClient client) + { + if (client is IAsyncDisposable asyncDisposable) + { + asyncDisposable.DisposeAsync().AsTask().GetAwaiter().GetResult(); + return; + } + + if (client is IDisposable disposable) + disposable.Dispose(); + } + internal sealed record DiscoveredMcpTool(string PluginId, ITool Tool, string Detail); private sealed record McpToolDescriptor(string LocalName, string RemoteName, string Description, string InputSchemaText); } diff --git a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs index 676aaf4..1695a3d 100644 --- a/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs +++ b/src/OpenClaw.Agent/Plugins/NativePluginRegistry.cs @@ -230,10 +230,28 @@ public void Dispose() foreach (var tool in _tools) { if (tool is IDisposable d) - d.Dispose(); + { + try + { + d.Dispose(); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to dispose native tool '{ToolName}' during registry shutdown", tool.Name); + } + } } foreach (var resource in _ownedResources) - resource.Dispose(); + { + try + { + resource.Dispose(); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to dispose owned native-plugin resource during registry shutdown"); + } + } } } diff --git a/src/OpenClaw.Agent/Tools/McpNativeTool.cs b/src/OpenClaw.Agent/Tools/McpNativeTool.cs index ce1a006..1a2bdb4 100644 --- a/src/OpenClaw.Agent/Tools/McpNativeTool.cs +++ b/src/OpenClaw.Agent/Tools/McpNativeTool.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using System.Text.Json.Serialization; using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; using OpenClaw.Core.Abstractions; @@ -50,13 +51,7 @@ public async ValueTask ExecuteAsync(string argumentsJson, CancellationTo argsDict[prop.Name] = value; } var response = await client.CallToolAsync(remoteName, argsDict, progress: null, cancellationToken: ct); - var parts = new List(); - foreach (var item in response.Content) - { - if (item is TextContentBlock t) - parts.Add(t.Text ?? ""); - } - var text = string.Join("\n\n", parts); + var text = FormatResponseContent(response); var isError = response.IsError ?? false; return isError ? $"Error: {text}" : text; } @@ -73,4 +68,50 @@ public async ValueTask ExecuteAsync(string argumentsJson, CancellationTo return $"Error: MCP tool '{localName}' failed: {ex.Message}"; } } + + private static string FormatResponseContent(CallToolResult response) + { + var parts = new List(); + + foreach (var item in response.Content ?? []) + { + switch (item) + { + case TextContentBlock textBlock when !string.IsNullOrEmpty(textBlock.Text): + parts.Add(textBlock.Text); + break; + case EmbeddedResourceBlock { Resource: TextResourceContents resource } when !string.IsNullOrEmpty(resource.Text): + parts.Add(resource.Text); + break; + default: + parts.Add(JsonSerializer.Serialize(item, McpToolSerializerContext.Default.ContentBlock)); + break; + } + } + + if (response.StructuredContent is { } structuredContent && + structuredContent.ValueKind is not (JsonValueKind.Undefined or JsonValueKind.Null)) + { + parts.Add(structuredContent.GetRawText()); + } + + return string.Join("\n\n", parts); + } } + +[JsonSerializable(typeof(ContentBlock))] +[JsonSerializable(typeof(TextContentBlock))] +[JsonSerializable(typeof(ImageContentBlock))] +[JsonSerializable(typeof(AudioContentBlock))] +[JsonSerializable(typeof(EmbeddedResourceBlock))] +[JsonSerializable(typeof(ResourceLinkBlock))] +[JsonSerializable(typeof(ToolUseContentBlock))] +[JsonSerializable(typeof(ToolResultContentBlock))] +[JsonSerializable(typeof(ResourceContents))] +[JsonSerializable(typeof(TextResourceContents))] +[JsonSerializable(typeof(BlobResourceContents))] +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + WriteIndented = false)] +internal sealed partial class McpToolSerializerContext : JsonSerializerContext; diff --git a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs index 52b7730..ed4f6cb 100644 --- a/src/OpenClaw.Tests/McpServerToolRegistryTests.cs +++ b/src/OpenClaw.Tests/McpServerToolRegistryTests.cs @@ -24,7 +24,7 @@ public sealed class McpServerToolRegistryTests : IAsyncDisposable [Fact] public async Task LoadAsync_HttpServer_DiscoversAndExecutesTools() { - var (serverUrl, calls) = await StartMcpServerAsync(); + var (serverUrl, calls) = await StartMcpServerAsync(); using var registry = new McpServerToolRegistry( new McpPluginsConfig { @@ -66,7 +66,7 @@ public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() Environment.SetEnvironmentVariable("TEST_AUTH_TOKEN", "secret-token-123"); try { - var (serverUrl, calls, receivedHeaders) = await StartMcpServerWithHeaderCheckAsync(); + var (serverUrl, calls, receivedHeaders) = await StartMcpServerWithHeaderCheckAsync(); using var registry = new McpServerToolRegistry( new McpPluginsConfig { @@ -105,10 +105,67 @@ public async Task LoadAsync_HttpServer_WithHeaders_ResolvesSecrets() } } + [Fact] + public async Task LoadAsync_HttpServer_WithStructuredContentOnly_ReturnsStructuredJson() + { + var (serverUrl, _) = await StartMcpServerAsync(); + using var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + var tool = Assert.Single(nativeRegistry.Tools); + var result = await tool.ExecuteAsync("{}", CancellationToken.None); + using var document = JsonDocument.Parse(result); + Assert.Equal(123, document.RootElement.GetProperty("value").GetInt32()); + Assert.Equal("ok", document.RootElement.GetProperty("status").GetString()); + } + + [Fact] + public async Task LoadAsync_HttpServer_WithImageContentBlock_ReturnsSerializedContent() + { + var (serverUrl, _) = await StartMcpServerAsync(); + using var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + var tool = Assert.Single(nativeRegistry.Tools); + var result = await tool.ExecuteAsync("{}", CancellationToken.None); + Assert.Contains("image/png", result, StringComparison.Ordinal); + Assert.Contains("type", result, StringComparison.OrdinalIgnoreCase); + } + [Fact] public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedResource() { - var (serverUrl, _) = await StartMcpServerAsync(); + var (serverUrl, _) = await StartMcpServerAsync(); using var registry = new McpServerToolRegistry( new McpPluginsConfig { @@ -138,7 +195,7 @@ public async Task RegisterToolsAsync_MultipleCalls_DoesNotRegisterSelfAsOwnedRes [Fact] public async Task LoadAsync_ConcurrentCalls_LoadsToolsOnce() { - var (serverUrl, calls) = await StartMcpServerAsync(); + var (serverUrl, calls) = await StartMcpServerAsync(); using var registry = new McpServerToolRegistry( new McpPluginsConfig { @@ -167,7 +224,7 @@ public async Task LoadAsync_ConcurrentCalls_LoadsToolsOnce() [Fact] public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() { - var (serverUrl, _) = await StartMcpServerAsync(); + var (serverUrl, _) = await StartMcpServerAsync(); var config = new McpPluginsConfig { Enabled = true, @@ -205,7 +262,7 @@ public async Task LoadAsync_WhenFirstAttemptFails_AllowsRetryAndLoadsTools() [Fact] public async Task LoadAsync_UsesRequestTimeoutForToolListing_NotStartupTimeout() { - var (serverUrl, _) = await StartMcpServerAsync(TimeSpan.FromSeconds(2)); + var (serverUrl, _) = await StartMcpServerAsync(TimeSpan.FromSeconds(2)); using var registry = new McpServerToolRegistry( new McpPluginsConfig { @@ -230,13 +287,61 @@ public async Task LoadAsync_UsesRequestTimeoutForToolListing_NotStartupTimeout() Assert.Equal("demo.echo", tool.Name); } + [Fact] + public async Task LoadAsync_AfterDispose_ThrowsObjectDisposedException() + { + using var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + }, + NullLogger.Instance); + + registry.Dispose(); + + await Assert.ThrowsAsync(() => registry.LoadAsync(CancellationToken.None)); + } + + [Fact] + public async Task Dispose_MayBeCalledTwice_AfterToolRegistration() + { + var (serverUrl, _) = await StartMcpServerAsync(); + var registry = new McpServerToolRegistry( + new McpPluginsConfig + { + Enabled = true, + Servers = new Dictionary(StringComparer.Ordinal) + { + ["demo"] = new() + { + Transport = "http", + Url = serverUrl + } + } + }, + NullLogger.Instance); + using var nativeRegistry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance, new ToolingConfig()); + + await registry.RegisterToolsAsync(nativeRegistry, CancellationToken.None); + + var ex = Record.Exception(() => + { + registry.Dispose(); + registry.Dispose(); + }); + + Assert.Null(ex); + } + public async ValueTask DisposeAsync() { foreach (var app in _apps) await app.DisposeAsync(); } - private async Task<(string ServerUrl, McpCallTracker Tracker)> StartMcpServerAsync(TimeSpan? toolsListDelay = null) + private async Task<(string ServerUrl, McpCallTracker Tracker)> StartMcpServerAsync(TimeSpan? toolsListDelay = null) + where TTools : class { var tracker = new McpCallTracker(); var builder = WebApplication.CreateSlimBuilder(); @@ -251,7 +356,7 @@ public async ValueTask DisposeAsync() }; }) .WithHttpTransport(options => { options.Stateless = true; }) - .WithTools(); + .WithTools(); var app = builder.Build(); app.Use(async (context, next) => { @@ -266,7 +371,8 @@ public async ValueTask DisposeAsync() return ($"{address.TrimEnd('/')}/mcp", tracker); } - private async Task<(string ServerUrl, McpCallTracker Tracker, Dictionary ReceivedHeaders)> StartMcpServerWithHeaderCheckAsync() + private async Task<(string ServerUrl, McpCallTracker Tracker, Dictionary ReceivedHeaders)> StartMcpServerWithHeaderCheckAsync() + where TTools : class { var tracker = new McpCallTracker(); var receivedHeaders = new Dictionary(StringComparer.OrdinalIgnoreCase); @@ -282,7 +388,7 @@ public async ValueTask DisposeAsync() }; }) .WithHttpTransport(options => { options.Stateless = true; }) - .WithTools(); + .WithTools(); var app = builder.Build(); app.Use(async (context, next) => { @@ -349,4 +455,26 @@ private sealed class DemoMcpTools public string Echo([Description("text")] string text) => $"demo:{text}"; } + + [McpServerToolType] + private sealed class StructuredMcpTools + { + [McpServerTool(Name = "structured", ReadOnly = true), Description("Structured response tool")] + public CallToolResult Structured() + => new() + { + StructuredContent = JsonSerializer.SerializeToElement(new { value = 123, status = "ok" }) + }; + } + + [McpServerToolType] + private sealed class BinaryMcpTools + { + [McpServerTool(Name = "image", ReadOnly = true), Description("Image response tool")] + public CallToolResult Image() + => new() + { + Content = [ImageContentBlock.FromBytes("png-bytes"u8.ToArray(), "image/png")] + }; + } } diff --git a/src/OpenClaw.Tests/NativePluginTests.cs b/src/OpenClaw.Tests/NativePluginTests.cs index 91b0d91..3c042ca 100644 --- a/src/OpenClaw.Tests/NativePluginTests.cs +++ b/src/OpenClaw.Tests/NativePluginTests.cs @@ -168,6 +168,40 @@ public void RegisterOwnedResource_SameInstanceTwice_DisposesOnce() Assert.Equal(1, resource.DisposeCalls); } + + [Fact] + public void Dispose_ToolDisposeFailure_DoesNotPreventOwnedResourceCleanup() + { + var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + var tool = new ThrowingDisposableFakeTool("dup_tool"); + var resource = new DisposableOwnedResource(); + + registry.RegisterExternalTool(tool, "mcp:test"); + registry.RegisterOwnedResource(resource); + + var ex = Record.Exception(() => registry.Dispose()); + + Assert.Null(ex); + Assert.Equal(1, tool.DisposeCalls); + Assert.Equal(1, resource.DisposeCalls); + } + + [Fact] + public void Dispose_OwnedResourceDisposeFailure_DoesNotAbortRemainingCleanup() + { + var registry = new NativePluginRegistry(new NativePluginsConfig(), NullLogger.Instance); + var first = new ThrowingOwnedResource(); + var second = new DisposableOwnedResource(); + + registry.RegisterOwnedResource(first); + registry.RegisterOwnedResource(second); + + var ex = Record.Exception(() => registry.Dispose()); + + Assert.Null(ex); + Assert.Equal(1, first.DisposeCalls); + Assert.Equal(1, second.DisposeCalls); + } } public class PluginPreferenceTests @@ -857,6 +891,16 @@ public void Dispose() => DisposeCalls++; } +file sealed class ThrowingOwnedResource : IDisposable +{ + public int DisposeCalls { get; private set; } + public void Dispose() + { + DisposeCalls++; + throw new InvalidOperationException("owned resource dispose failed"); + } +} + /// Minimal ILogger for tests. file sealed class NullLogger : Microsoft.Extensions.Logging.ILogger {