diff --git a/changelog.d/unreleased/1417.fixed.md b/changelog.d/unreleased/1417.fixed.md new file mode 100644 index 0000000000..bde6af555c --- /dev/null +++ b/changelog.d/unreleased/1417.fixed.md @@ -0,0 +1,17 @@ +--- +category: fixed +issues: + - 1417 +affected: + - src/CodeIndex/Mcp/McpServer.cs + - src/CodeIndex/Mcp/McpToolHandlers.cs + - tests/CodeIndex.Tests/McpServerTests.cs +--- + +## English + +- **MCP tool argument type mismatches now return JSON-RPC invalid params (#1417)** — wrong JSON types such as a string `limit` now produce `-32602` with structured parameter details instead of falling through to an internal/tool failure. + +## 日本語 + +- **MCP ツール引数の型不一致が JSON-RPC invalid params を返すようになりました (#1417)** — 文字列の `limit` など誤った JSON 型は、internal/tool failure に落ちず `-32602` と構造化されたパラメータ詳細を返します。 diff --git a/changelog.d/unreleased/1469.fixed.md b/changelog.d/unreleased/1469.fixed.md new file mode 100644 index 0000000000..0b46b553a2 --- /dev/null +++ b/changelog.d/unreleased/1469.fixed.md @@ -0,0 +1,16 @@ +--- +category: fixed +issues: + - 1469 +affected: + - src/CodeIndex/Mcp/McpServer.cs + - tests/CodeIndex.Tests/McpServerTests.cs +--- + +## English + +- **MCP startup logs no longer expose the full DB path by default (#1469)** — the startup banner now logs only a sanitized DB filename unless `CDIDX_DEBUG=unsafe` is set. + +## 日本語 + +- **MCP 起動ログが既定で完全な DB パスを公開しないようになりました (#1469)** — 起動バナーは `CDIDX_DEBUG=unsafe` が設定されていない限り、サニタイズ済みの DB ファイル名だけを記録します。 diff --git a/changelog.d/unreleased/1470.fixed.md b/changelog.d/unreleased/1470.fixed.md new file mode 100644 index 0000000000..e935eb49b9 --- /dev/null +++ b/changelog.d/unreleased/1470.fixed.md @@ -0,0 +1,16 @@ +--- +category: fixed +issues: + - 1470 +affected: + - src/CodeIndex/Mcp/McpServer.cs + - tests/CodeIndex.Tests/McpServerTests.cs +--- + +## English + +- **MCP catch-all error responses now hide exception details by default (#1470)** — unexpected tool and loop failures return generic wire messages while preserving detailed diagnostics in stderr, with verbose responses limited to `CDIDX_DEBUG=unsafe`. + +## 日本語 + +- **MCP catch-all エラー応答が既定で例外詳細を隠すようになりました (#1470)** — 予期しないツール/ループ失敗は wire 上では汎用メッセージを返し、詳細診断は stderr に残します。詳細応答は `CDIDX_DEBUG=unsafe` の場合だけ有効です。 diff --git a/src/CodeIndex/Mcp/McpServer.cs b/src/CodeIndex/Mcp/McpServer.cs index fd088e24a2..b736e7dcf2 100644 --- a/src/CodeIndex/Mcp/McpServer.cs +++ b/src/CodeIndex/Mcp/McpServer.cs @@ -157,6 +157,7 @@ public partial class McpServer : IDisposable internal const int DefaultMaxResponseBytes = 10 * 1024 * 1024; private const string MaxResponseBytesEnvVar = "CDIDX_MCP_RESPONSE_MAX_BYTES"; private const string KeepAliveIntervalEnvironmentVariable = "CDIDX_MCP_KEEP_ALIVE_INTERVAL_S"; + internal const string DebugEnvironmentVariable = "CDIDX_DEBUG"; internal const int MaxJsonDepth = 32; internal const int MaxBatchRequestCount = 100; // Stdio buffer for the JSON-RPC loop. Sized to fit typical large MCP payloads (e.g. batch_query) @@ -436,7 +437,7 @@ internal async Task RunAsync(IMcpTransport transport, CancellationToken cancella // Use stderr for logging so stdout stays clean for JSON-RPC // stdoutをJSON-RPC用にクリーンに保つため、ログはstderrに出力 - ConsoleUi.TryWriteErrorLine($"[cdidx-mcp] Starting MCP server v{_version} (db: {_dbPath}, transport: {transport.Name} @ {transport.Endpoint}, max in-flight: {MaxConcurrency})"); + ConsoleUi.TryWriteErrorLine($"[cdidx-mcp] Starting MCP server v{_version} (db: {FormatDbPathForLog(_dbPath)}, transport: {transport.Name} @ {transport.Endpoint}, max in-flight: {MaxConcurrency})"); if (transport is HttpMcpTransport httpTransport) { @@ -2143,11 +2144,25 @@ private async Task HandleToolsCallAsync(JsonNode? id, JsonNode? callPa if (ValidateToolArguments(toolName, args) is JsonObject argumentError) { metricsError = "invalid_argument"; - response = CreateToolErrorResponse(id, argumentError["message"]!.GetValue(), - category: McpErrorEnvelope.CategoryInvalidArgument, - suggestion: "Use exactly the argument names advertised by tools/list for this tool.", - retrySafe: false, - extraData: argumentError); + if (argumentError["jsonrpc_invalid_params"] is JsonValue invalidParamsMarker + && invalidParamsMarker.TryGetValue(out var invalidParams) + && invalidParams) + { + argumentError.Remove("jsonrpc_invalid_params"); + response = CreateErrorResponse(hasId: true, id: id, code: -32602, message: argumentError["message"]!.GetValue(), + category: McpErrorEnvelope.CategoryInvalidArgument, + suggestion: "Use the JSON types advertised by tools/list for this tool.", + retrySafe: false, + extraData: argumentError); + } + else + { + response = CreateToolErrorResponse(id, argumentError["message"]!.GetValue(), + category: McpErrorEnvelope.CategoryInvalidArgument, + suggestion: "Use exactly the argument names advertised by tools/list for this tool.", + retrySafe: false, + extraData: argumentError); + } } else if (ValidateCommonListArguments(args) is JsonObject listArgumentError) { @@ -2556,6 +2571,28 @@ internal static string BuildUnknownNotificationLog(string method) => internal static bool IsSupportedMcpLogLevel(string? level) => level is "debug" or "info" or "notice" or "warning" or "error" or "critical" or "alert" or "emergency"; + internal static bool IsUnsafeDebugEnabled() + => string.Equals(Environment.GetEnvironmentVariable(DebugEnvironmentVariable), "unsafe", StringComparison.OrdinalIgnoreCase); + + internal static string FormatDbPathForLog(string dbPath) + { + if (IsUnsafeDebugEnabled()) + return dbPath; + + try + { + var path = dbPath; + if (Uri.TryCreate(dbPath, UriKind.Absolute, out var uri) && uri.IsFile) + path = uri.LocalPath; + var fileName = Path.GetFileName(path); + return string.IsNullOrWhiteSpace(fileName) ? "(configured db)" : fileName; + } + catch + { + return "(configured db)"; + } + } + // Wire-safe error body for the tool catch-all. Mentions the tool and the // exception type so the client can branch (retry vs. surface to user) // while keeping bound values or matched content out of the response (#1530). @@ -2569,6 +2606,8 @@ internal static bool IsSupportedMcpLogLevel(string? level) // #1530 で封じた ex.Message 漏れを再現させずに失敗詳細をクライアントへ届ける。 internal static string BuildSanitizedToolErrorMessage(string toolName, Exception ex) { + if (!IsUnsafeDebugEnabled()) + return $"Tool '{toolName}' failed. See cdidx server stderr for details."; if (ex is CodeIndexException codeIndexEx) return $"Error executing {toolName} ({ex.GetType().Name}) [{codeIndexEx.Code}/{codeIndexEx.Category}]{BuildPathFragment(codeIndexEx)}{BuildHintFragment(codeIndexEx)}. See cdidx server stderr for details."; return $"Error executing {toolName} ({ex.GetType().Name}). See cdidx server stderr for details."; @@ -2579,6 +2618,8 @@ internal static string BuildSanitizedToolErrorMessage(string toolName, Exception // JSON-RPC ループ catch-all のワイヤー向け本文。理由はツール catch-all と同じ（#1530, #1580）。 internal static string BuildSanitizedLoopErrorMessage(Exception ex) { + if (!IsUnsafeDebugEnabled()) + return "Internal MCP error. See cdidx server stderr for details."; if (ex is CodeIndexException codeIndexEx) return $"Internal error ({ex.GetType().Name}) [{codeIndexEx.Code}/{codeIndexEx.Category}]{BuildPathFragment(codeIndexEx)}{BuildHintFragment(codeIndexEx)}. See cdidx server stderr for details."; return $"Internal error ({ex.GetType().Name}). See cdidx server stderr for details."; diff --git a/src/CodeIndex/Mcp/McpToolHandlers.cs b/src/CodeIndex/Mcp/McpToolHandlers.cs index 50de8ef1c4..3c07ae5c59 100644 --- a/src/CodeIndex/Mcp/McpToolHandlers.cs +++ b/src/CodeIndex/Mcp/McpToolHandlers.cs @@ -386,9 +386,88 @@ private static List ReadStringList(JsonNode? args, string propertyName) } } + if (ValidateToolArgumentTypes(toolName, obj) is JsonObject typeError) + return typeError; + return null; } + private static JsonObject? ValidateToolArgumentTypes(string toolName, JsonObject args) + { + foreach (var property in args) + { + if (TryGetExpectedJsonType(toolName, property.Key, out var expected) + && !MatchesExpectedJsonType(property.Value, expected)) + { + return new JsonObject + { + ["message"] = $"Invalid type for argument '{property.Key}' on tool '{toolName}'. Expected {expected}.", + ["tool"] = toolName, + ["parameter"] = property.Key, + ["expected"] = expected, + ["actual"] = DescribeJsonType(property.Value), + ["jsonrpc_invalid_params"] = true, + }; + } + } + + return null; + } + + private static bool TryGetExpectedJsonType(string toolName, string argumentName, out string expected) + { + if (argumentName is "path" or "project" or "excludePaths" or "names") + { + expected = string.Empty; + return false; + } + + expected = argumentName switch + { + "limit" or "offset" or "snippetLines" or "maxLineWidth" or "before" or "after" or + "focusLine" or "focusColumn" or "focusLength" or "startLine" or "endLine" or + "maxHops" or "maxDepth" or "depth" or "parallelism" => "integer", + "excludeTests" or "includeGenerated" or "rawQuery" or "noDedup" or "exactSubstring" or + "exactName" or "exact" or "prefix" or "countOnly" or "includeBody" or "lsp_compatible" or + "regex" or "withPaths" or "rebuild" or "dryRun" or "dry_run" or "force" or "optimize" => "boolean", + "query" or "lang" or "kind" or "format" or "rankBy" or "since" or "path" or "project" or + "solution" or "symbol" or "direction" or "groupBy" or "category" or "language" or + "description" or "context" or "toolInvocationContext" or "db" => "string", + "queries" => "array", + _ => string.Empty, + }; + + if (expected.Length == 0) + return false; + + return true; + } + + private static bool MatchesExpectedJsonType(JsonNode? node, string expected) => expected switch + { + "integer" => node is JsonValue value && value.TryGetValue(out _), + "boolean" => node is JsonValue value && value.TryGetValue(out _), + "string" => node is JsonValue value && value.TryGetValue(out _), + "array" => node is JsonArray, + _ => true, + }; + + private static string DescribeJsonType(JsonNode? node) + { + if (node is null) + return "null"; + return node.GetValueKind() switch + { + JsonValueKind.String => "string", + JsonValueKind.Number => "number", + JsonValueKind.True or JsonValueKind.False => "boolean", + JsonValueKind.Array => "array", + JsonValueKind.Object => "object", + JsonValueKind.Null => "null", + _ => "unknown", + }; + } + private static bool IsKnownToolName(string toolName) => toolName switch { "search" or "definition" or "references" or "callers" or "callees" or "symbols" or diff --git a/tests/CodeIndex.Tests/McpServerTests.cs b/tests/CodeIndex.Tests/McpServerTests.cs index cf6e34e116..bd028c52d8 100644 --- a/tests/CodeIndex.Tests/McpServerTests.cs +++ b/tests/CodeIndex.Tests/McpServerTests.cs @@ -478,6 +478,43 @@ public async Task RunAsync_InitializeEmitsInitializedNotificationAfterResponseOn Assert.Equal(2, JsonNode.Parse(transport.WrittenFrames[2])!["id"]!.GetValue()); } + [Fact] + public async Task RunAsync_StartupLogSanitizesDbPathByDefault_Issue1469() + { + using var error = new StringWriter(); + var previousDebug = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, null); + await Task.Run(() => + { + lock (TestConsoleLock.Gate) + { + var previousError = Console.Error; + try + { + Console.SetError(error); +#pragma warning disable xUnit1031 + _server.RunAsync(new QueueMcpTransport(), CancellationToken.None).GetAwaiter().GetResult(); +#pragma warning restore xUnit1031 + } + finally + { + Console.SetError(previousError); + } + } + }); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previousDebug); + } + + var log = error.ToString(); + Assert.Contains("db: " + Path.GetFileName(_dbPath), log); + Assert.DoesNotContain(_dbPath, log); + } + [Fact] public void Initialize_AdvertisesResourcesAndPrompts() { @@ -983,6 +1020,23 @@ public void ToolCall_ResponseShape_HasStableMcpResultEnvelope(string toolName, s Assert.NotNull(result["_meta"]!["correlation_id"]); } + [Fact] + public void ToolCall_TypeMismatch_ReturnsInvalidParams_Issue1417() + { + var request = JsonNode.Parse( + """{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"search","arguments":{"query":"Run","limit":"not-an-int"}}}""")!; + + var response = _server.HandleMessage(request)!; + + Assert.Null(response["result"]); + var error = response["error"]!; + Assert.Equal(-32602, error["code"]!.GetValue()); + Assert.Equal("invalid_argument", error["data"]!["category"]!.GetValue()); + Assert.Equal("limit", error["data"]!["parameter"]!.GetValue()); + Assert.Equal("integer", error["data"]!["expected"]!.GetValue()); + Assert.Equal("string", error["data"]!["actual"]!.GetValue()); + } + [Fact] public void Initialize_ReturnsInstructions() { @@ -1408,13 +1462,23 @@ public void BuildSanitizedToolErrorMessage_OmitsExceptionMessage() // exception type should reach the wire; full detail stays in stderr. var ex = new InvalidOperationException("near 'SECRET_LITERAL': syntax error"); - var message = McpServer.BuildSanitizedToolErrorMessage("search", ex); + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + string message; + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, null); + message = McpServer.BuildSanitizedToolErrorMessage("search", ex); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } - Assert.Contains("Error executing search", message); - Assert.Contains(nameof(InvalidOperationException), message); + Assert.Equal("Tool 'search' failed. See cdidx server stderr for details.", message); Assert.Contains("server stderr", message); Assert.DoesNotContain("SECRET_LITERAL", message); Assert.DoesNotContain("syntax error", message); + Assert.DoesNotContain(nameof(InvalidOperationException), message); } [Fact] @@ -1424,13 +1488,44 @@ public void BuildSanitizedLoopErrorMessage_OmitsExceptionMessage() // outer JSON-RPC loop catch-all (#1530). var ex = new InvalidOperationException("PRAGMA failed: secret table 'leaky_table' missing"); - var message = McpServer.BuildSanitizedLoopErrorMessage(ex); + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + string message; + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, null); + message = McpServer.BuildSanitizedLoopErrorMessage(ex); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } - Assert.Contains("Internal error", message); - Assert.Contains(nameof(InvalidOperationException), message); + Assert.Equal("Internal MCP error. See cdidx server stderr for details.", message); Assert.Contains("server stderr", message); Assert.DoesNotContain("leaky_table", message); Assert.DoesNotContain("PRAGMA failed", message); + Assert.DoesNotContain(nameof(InvalidOperationException), message); + } + + [Fact] + public void BuildSanitizedToolErrorMessage_UnsafeDebugIncludesExceptionType() + { + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, "unsafe"); + var ex = new InvalidOperationException("near 'SECRET_LITERAL': syntax error"); + + var message = McpServer.BuildSanitizedToolErrorMessage("search", ex); + + Assert.Contains("Error executing search", message); + Assert.Contains(nameof(InvalidOperationException), message); + Assert.DoesNotContain("SECRET_LITERAL", message); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } } [Fact] @@ -1449,7 +1544,17 @@ public void BuildSanitizedToolErrorMessage_CodeIndexException_EchoesStructuredFi path: "/var/cdidx/state.db", hint: "Close other cdidx invocations."); - var message = McpServer.BuildSanitizedToolErrorMessage("search", ex); + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + string message; + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, "unsafe"); + message = McpServer.BuildSanitizedToolErrorMessage("search", ex); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } Assert.Contains("Error executing search", message); Assert.Contains(nameof(CodeIndexException), message); @@ -1638,7 +1743,17 @@ public void BuildSanitizedLoopErrorMessage_CodeIndexException_EchoesStructuredFi path: "/var/cdidx/state.db", hint: "Close other cdidx invocations."); - var message = McpServer.BuildSanitizedLoopErrorMessage(ex); + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + string message; + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, "unsafe"); + message = McpServer.BuildSanitizedLoopErrorMessage(ex); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } Assert.Contains("Internal error", message); Assert.Contains(nameof(CodeIndexException), message); @@ -1656,7 +1771,17 @@ public void BuildSanitizedToolErrorMessage_CodeIndexException_NoPathNoHint_Omits category: CodeIndexExceptionCategory.Database, message: "Generic failure."); - var message = McpServer.BuildSanitizedToolErrorMessage("status", ex); + var previous = Environment.GetEnvironmentVariable(McpServer.DebugEnvironmentVariable); + string message; + try + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, "unsafe"); + message = McpServer.BuildSanitizedToolErrorMessage("status", ex); + } + finally + { + Environment.SetEnvironmentVariable(McpServer.DebugEnvironmentVariable, previous); + } Assert.Contains("[E008_DB_ERROR/database]", message); Assert.DoesNotContain("path=", message);