diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs index ef0cb3a6b..047a86941 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpClientManagerStatusTests.cs @@ -126,6 +126,39 @@ public void BuildConnectionFailureStatus_ForNetworkFailure_ReturnsUnreachable() Assert.Equal(ErrorAt, status.LastErrorAt); } + [Fact] + public void BuildConnectionFailureStatus_ForStdioSpawnFailureWithEmbeddedStatusLikeDigits_ReturnsUnreachable() + { + var entry = new McpServerEntry + { + Transport = "stdio", + Command = "netclaw-missing-mcp-server-632401b4aa2f4c1e9c1b2a3d4e5f6789", + Enabled = true, + }; + + // A stdio process-spawn failure carries the command name inside its message. The + // command name is caller-supplied config data, not an HTTP signal, and can + // coincidentally embed digits that look like a status code -- here "401" inside + // the GUID suffix. No HTTP request ever occurs for stdio, so this must never be + // misread as an HTTP 401 failure. + var spawnFailure = new IOException( + $"An error occurred trying to start process '{entry.Command}' with working " + + "directory '/tmp'. No such file or directory"); + + var status = McpClientManager.BuildConnectionFailureStatus( + new McpServerName("notifications"), + entry, + spawnFailure, + hasCachedTokens: false, + hasOAuthRuntimeHints: false, + ErrorAt); + + Assert.Equal(McpConnectionState.Unreachable, status.State); + Assert.Equal("Failed to reach MCP server. Check daemon logs for details.", status.ErrorMessage); + Assert.DoesNotContain("401", status.ErrorMessage, StringComparison.Ordinal); + Assert.Equal(ErrorAt, status.LastErrorAt); + } + [Fact] public void PublicErrorsNeverIncludeProviderBodySecrets() { diff --git a/src/Netclaw.Daemon.Tests/Mcp/McpSdkCatalogNotificationIntegrationTests.cs b/src/Netclaw.Daemon.Tests/Mcp/McpSdkCatalogNotificationIntegrationTests.cs index 6ebe7b4c4..26578ae41 100644 --- a/src/Netclaw.Daemon.Tests/Mcp/McpSdkCatalogNotificationIntegrationTests.cs +++ b/src/Netclaw.Daemon.Tests/Mcp/McpSdkCatalogNotificationIntegrationTests.cs @@ -53,6 +53,11 @@ public async Task RawListenAcknowledgementAndToolEvent_RefreshLiveCatalog() [Fact] public async Task FailedStdioStartup_IsReportedBeforeLeaseAssertions() { + // The GUID suffix on this command name is intentional and must stay random: it + // regression-guards McpClientManager.FindHttpStatus against sniffing a status code + // out of caller-supplied data. A prior bug made a bare Contains("401")/Contains("403") + // check misclassify a random GUID substring (e.g. "632401b4...") as an HTTP auth + // failure. See McpClientManagerStatusTests for the targeted unit coverage. var entry = new McpServerEntry { Transport = "stdio", diff --git a/src/Netclaw.Daemon/Mcp/McpClientManager.cs b/src/Netclaw.Daemon/Mcp/McpClientManager.cs index 33abe26c2..ea3cb9b36 100644 --- a/src/Netclaw.Daemon/Mcp/McpClientManager.cs +++ b/src/Netclaw.Daemon/Mcp/McpClientManager.cs @@ -1326,9 +1326,16 @@ internal static McpServerStatus BuildConnectionFailureStatus( bool hasOAuthRuntimeHints, DateTimeOffset errorAt) { + // A stdio server is a local child process: no HTTP request is ever made for it, so + // no HTTP status can genuinely appear in its failure. The spawn-failure message + // routinely embeds caller-supplied data -- the command name, a working directory -- + // that can coincidentally look like a status code (e.g. a GUID substring landing on + // "401"), so status sniffing is skipped outright for this transport. + var isStdioTransport = entry.Transport is "stdio"; + if (IsAuthFailure(ex)) { - if (!hasCachedTokens && entry.Transport is not "stdio" && hasOAuthRuntimeHints) + if (!hasCachedTokens && !isStdioTransport && hasOAuthRuntimeHints) { // "Awaiting auth" -- which sends the operator to `netclaw mcp auth` -- is only // correct for a genuine OAuth challenge: a Bearer WWW-Authenticate response (or @@ -1340,7 +1347,7 @@ internal static McpServerStatus BuildConnectionFailureStatus( // carrying the HTTP status instead. return IsOAuthChallenge(ex) ? CreateAwaitingAuthStatus(serverName, errorAt) - : CreateUnreachableStatus(serverName, ex, errorAt); + : CreateUnreachableStatus(serverName, ex, errorAt, isStdioTransport); } return CreateAuthFailedStatus( @@ -1350,7 +1357,7 @@ internal static McpServerStatus BuildConnectionFailureStatus( errorAt); } - return CreateUnreachableStatus(serverName, ex, errorAt); + return CreateUnreachableStatus(serverName, ex, errorAt, isStdioTransport); } internal static McpServerStatus CreateAwaitingAuthStatus( @@ -1387,17 +1394,18 @@ internal static McpServerStatus CreateAuthFailedStatus( internal static McpServerStatus CreateUnreachableStatus( McpServerName serverName, Exception ex, - DateTimeOffset errorAt) + DateTimeOffset errorAt, + bool isStdioTransport) => new( serverName, McpConnectionState.Unreachable, 0, - GetSafeConnectionFailure(ex), + GetSafeConnectionFailure(ex, isStdioTransport), errorAt); - private static string GetSafeConnectionFailure(Exception ex) + private static string GetSafeConnectionFailure(Exception ex, bool isStdioTransport) { - var status = FindHttpStatus(ex); + var status = isStdioTransport ? null : FindHttpStatus(ex); if (status is not null) return $"MCP server request failed (HTTP {(int)status.Value} {status.Value})."; if (ex is TimeoutException or TaskCanceledException) @@ -1625,6 +1633,17 @@ private static IEnumerable EnumerateExceptionTree(Exception root) } } + /// + /// Reads an HTTP status from an exception chain. Only two anchored shapes are trusted: a + /// typed , and the literal "status {Name}" / + /// "HTTP {code}" text the MCP SDK and .NET's own HttpClient use when they report one + /// (see the SDK's HttpResponseMessageExtensions.CreateHttpRequestException and + /// , both of which set the typed status too). A bare + /// digit or word match (e.g. Contains("401")) is deliberately not used: exception + /// messages routinely embed caller-supplied data -- command names, file paths, GUIDs -- and + /// a coincidental "401"/"403" substring there would misreport an unrelated failure as an + /// HTTP auth rejection. + /// private static HttpStatusCode? FindHttpStatus(Exception ex) { if (ex is HttpRequestException { StatusCode: { } status }) @@ -1635,12 +1654,6 @@ private static IEnumerable EnumerateExceptionTree(Exception root) || ex.Message.Contains($"HTTP {(int)candidate}", StringComparison.OrdinalIgnoreCase)) return candidate; } - if (ex.Message.Contains("403", StringComparison.Ordinal) - || ex.Message.Contains("Forbidden", StringComparison.OrdinalIgnoreCase)) - return HttpStatusCode.Forbidden; - if (ex.Message.Contains("401", StringComparison.Ordinal) - || ex.Message.Contains("Unauthorized", StringComparison.OrdinalIgnoreCase)) - return HttpStatusCode.Unauthorized; return ex.InnerException is null ? null : FindHttpStatus(ex.InnerException); }