From a6576623089f11b2119dc4cc61123ae124d89501 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Tue, 29 Sep 2026 14:57:41 +0200 Subject: [PATCH 1/2] Fix MCP null argument binding and history formatting Treat explicit JSON null option and parameter values as omitted before binding, so nullable value-typed options such as watch --interval no longer fail with "Invalid value". A null continuation token is still rejected because it signals an exhausted result set. Render echoed history values with the invariant culture so recorded command lines stay replayable, and make the echo MCP test independent of other tests that record the same history line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ToolOperationsCallToolTests.cs | 95 ++++++++++++++++++- .../ToolOperations.cs | 29 +++++- docs/mcp.md | 2 + 3 files changed, 119 insertions(+), 7 deletions(-) diff --git a/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs b/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs index 531e6836..c10d8d6e 100644 --- a/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs +++ b/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs @@ -5,6 +5,7 @@ namespace CosmosShell.Tests; using System.Collections.Generic; +using System.Globalization; using System.Linq; using System.Runtime.CompilerServices; using System.Text.Json; @@ -346,6 +347,93 @@ public void FormatPositionalsForHistory_RendersContiguousValuesAndExpandsArrays( Assert.Equal(" \"hello\" \"world\"", ToolOperations.FormatPositionalsForHistory(echoParameters, echoValues)); } + [Fact] + public void HistoryFormatting_UsesInvariantCultureForOptionsAndPositionals() + { + var originalCulture = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("de-DE"); + + var intervalOption = ShellInterpreter.Instance.App.Commands["watch"].Options.Single(o => o.Name[0] == "interval"); + Assert.Equal(" --interval \"1.5\"", ToolOperations.FormatOptionForHistory(intervalOption, 1.5d)); + + var echoParameters = ShellInterpreter.Instance.App.Commands["echo"].Parameters; + var echoValues = new Dictionary { [echoParameters[0]] = new object?[] { 1.5d, 2.5d } }; + Assert.Equal(" \"1.5\" \"2.5\"", ToolOperations.FormatPositionalsForHistory(echoParameters, echoValues)); + } + finally + { + CultureInfo.CurrentCulture = originalCulture; + } + } + + [Fact] + public async Task CallTool_NullNullableValueOption_IsOmitted() + { + var tool = CreateToolOperations(); + var shell = ShellInterpreter.Instance; + var originalState = shell.State; + var arguments = new Dictionary + { + ["query"] = Json("\"SELECT * FROM c\""), + ["max"] = Json("null"), + }; + + try + { + shell.State = new DisconnectedState(); + + var result = await tool.CallToolHandler(CallContext("query", arguments), CancellationToken.None); + + var (isError, root, document) = ReadResult(result); + using (document) + { + Assert.True(isError); + Assert.DoesNotContain("Invalid value", root.GetProperty("error").GetString(), StringComparison.Ordinal); + } + + Assert.Equal("query \"SELECT * FROM c\"", shell.History.ToArray()[^1]); + } + finally + { + shell.State = originalState; + } + } + + [Fact] + public async Task CallTool_NullStringOption_IsOmitted() + { + var tool = CreateToolOperations(); + var shell = ShellInterpreter.Instance; + var originalState = shell.State; + var arguments = new Dictionary + { + ["query"] = Json("\"SELECT * FROM c\""), + ["database"] = Json("null"), + }; + + try + { + shell.State = new DisconnectedState(); + + var result = await tool.CallToolHandler(CallContext("query", arguments), CancellationToken.None); + + var (isError, root, document) = ReadResult(result); + using (document) + { + Assert.True(isError); + Assert.DoesNotContain("Invalid value", root.GetProperty("error").GetString(), StringComparison.Ordinal); + } + + Assert.Equal("query \"SELECT * FROM c\"", shell.History.ToArray()[^1]); + } + finally + { + shell.State = originalState; + } + } + [Fact] public async Task CallTool_UnknownArgument_ReturnsErrorListingKnownArguments() { @@ -508,9 +596,10 @@ public async Task CallTool_EchoCommand_ReturnsSuccessResult() var tool = CreateToolOperations(); var history = ShellInterpreter.Instance.History.ToArray(); using var output = new StringWriter(); + var message = "hello-" + Guid.NewGuid().ToString("N"); var arguments = new Dictionary { - ["messages"] = Json("[\"hello\", \"world\"]"), + ["messages"] = Json($"[\"{message}\", \"world\"]"), }; var saved = AnsiConsole.Console; @@ -528,14 +617,14 @@ public async Task CallTool_EchoCommand_ReturnsSuccessResult() Assert.Contains("echo", output.ToString(), StringComparison.Ordinal); var recorded = ShellInterpreter.Instance.History.ToArray(); Assert.Equal(history.Length + 1, recorded.Length); - Assert.Equal("echo \"hello\" \"world\"", recorded[^1]); + Assert.Equal($"echo \"{message}\" \"world\"", recorded[^1]); Assert.Single(recorded, entry => entry == recorded[^1]); var (isError, root, document) = ReadResult(result); using (document) { Assert.False(isError); - Assert.Equal("hello world", root.GetProperty("result").GetString()); + Assert.Equal($"{message} world", root.GetProperty("result").GetString()); Assert.True(root.TryGetProperty("currentLocation", out _)); } } diff --git a/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs b/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs index ba049def..e409256b 100644 --- a/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs +++ b/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs @@ -4,6 +4,7 @@ namespace Azure.Data.Cosmos.Shell.Mcp; +using System.Globalization; using System.Reflection; using System.Text; using System.Text.Json; @@ -206,7 +207,7 @@ internal static bool MatchesArgumentName(string[] names, string? argumentName) internal static string FormatOptionForHistory(Option option, object? value) { - return $" --{option.Name[0]} {ShellLiteral.Quote(value?.ToString())}"; + return $" --{option.Name[0]} {ShellLiteral.Quote(FormatValueForHistory(value))}"; } // Shell syntax cannot skip a positional, so a later value would bind to the omitted slot on replay. @@ -243,18 +244,23 @@ internal static string FormatPositionalsForHistory(IReadOnlyList para { foreach (var element in array) { - sb.Append(' ').Append(ShellLiteral.Quote(element?.ToString())); + sb.Append(' ').Append(ShellLiteral.Quote(FormatValueForHistory(element))); } } else { - sb.Append(' ').Append(ShellLiteral.Quote(value?.ToString())); + sb.Append(' ').Append(ShellLiteral.Quote(FormatValueForHistory(value))); } } return sb.ToString(); } + private static string? FormatValueForHistory(object? value) + { + return Convert.ToString(value, CultureInfo.InvariantCulture); + } + private static bool IsPositionalSupplied(IReadOnlyDictionary values, Parameter parameter) { return values.TryGetValue(parameter, out var value) @@ -406,6 +412,11 @@ private static bool RequiresConfirmation(CommandFactory command) && annotation.Destructive; } + private static bool IsExplicitJsonNull(JsonElement value) + { + return value.ValueKind is JsonValueKind.Null; + } + private CallToolResult? BindMember( object cmd, PropertyInfo property, @@ -426,7 +437,7 @@ private static bool RequiresConfirmation(CommandFactory command) { convertedValue = rawValue is JsonElement jsonElement ? ConvertJsonElement(jsonElement, targetType) - : Convert.ChangeType(rawValue, targetType); + : Convert.ChangeType(rawValue, targetType, CultureInfo.InvariantCulture); } catch (Exception ex) { @@ -526,6 +537,11 @@ private async ValueTask OnCallToolsAsync( var option = command.Options.FirstOrDefault(a => MatchesArgumentName(a.Name, par.Key)); if (option != null) { + if (IsExplicitJsonNull(par.Value)) + { + continue; + } + var bindError = this.BindMember( cmd, option.PropertyInfo, @@ -545,6 +561,11 @@ private async ValueTask OnCallToolsAsync( var parameter = command.Parameters.FirstOrDefault(a => MatchesArgumentName(a.Name, par.Key)); if (parameter != null) { + if (IsExplicitJsonNull(par.Value)) + { + continue; + } + var bindError = this.BindMember( cmd, parameter.PropertyInfo, diff --git a/docs/mcp.md b/docs/mcp.md index 94126d2e..46878ea7 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -97,6 +97,8 @@ MCP tool invocations are echoed as command lines in the shell window, so anyone Positional arguments must be supplied without gaps: a call that provides a positional parameter while omitting an earlier one is rejected, because the equivalent shell command line would bind the value to the omitted slot. +Explicit `null` argument values are treated as omitted and are not echoed into shell history. The paging `continuation` argument is the exception: a `null` token means the result set is exhausted, so passing it back is rejected. + Your MCP client may use a remote LLM. Command outputs, query results, and file contents could be transmitted to external services. **Treat all shell output as potentially shared.** ### Best Practices From 05a5921322c4f9a7a401a0eba027f223f0642bec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Kr=C3=BCger?= Date: Fri, 2 Oct 2026 12:33:35 +0200 Subject: [PATCH 2/2] Clarify null continuation tokens for incomplete MCP results Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CosmosDBShell.Tests/ToolOperationsTests.cs | 4 ++++ CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs | 2 +- docs/mcp.md | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CosmosDBShell.Tests/ToolOperationsTests.cs b/CosmosDBShell.Tests/ToolOperationsTests.cs index 990246f5..63a067f9 100644 --- a/CosmosDBShell.Tests/ToolOperationsTests.cs +++ b/CosmosDBShell.Tests/ToolOperationsTests.cs @@ -159,6 +159,10 @@ public void GetTool_ExposesContinuationWithoutMakingItAShellOption(string comman var continuation = schema.GetProperty("properties").GetProperty("continuation"); Assert.Equal("string", continuation.GetProperty("type").GetString()); + var description = continuation.GetProperty("description").GetString(); + Assert.Contains("unless resultIncomplete is true", description); + Assert.Contains("truncated and cannot be resumed", description); + Assert.Contains("Never pass back a null token", description); } [Fact] diff --git a/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs b/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs index a62dba94..5389c86a 100644 --- a/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs +++ b/CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs @@ -32,7 +32,7 @@ internal class ToolOperations "The shell context changed while awaiting confirmation. Nothing was executed. Retry the command and confirm its current target."; private const string ContinuationDescription = - "Non-null continuation token returned by a previous call to this tool. Pass it back to fetch the next page, or omit this argument to start from the beginning. A null output token means the result is exhausted and no further call should be made. The value is opaque; do not modify it."; + "Non-null continuation token returned by a previous call to this tool. Pass it back to fetch the next page, or omit this argument to start from the beginning. A null output token means the result is exhausted unless resultIncomplete is true, in which case results were truncated and cannot be resumed. Never pass back a null token. For incomplete results, retry with a larger max or a narrower query. The value is opaque; do not modify it."; private readonly ILogger logger; private readonly LocationResourceSubscriptions locationSubscriptions; diff --git a/docs/mcp.md b/docs/mcp.md index f1ffbf96..a5c7d9fc 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -114,7 +114,7 @@ MCP tool invocations are echoed as command lines in the shell window, so anyone Positional arguments must be supplied without gaps: a call that provides a positional parameter while omitting an earlier one is rejected, because the equivalent shell command line would bind the value to the omitted slot. -Explicit `null` argument values are treated as omitted and are not echoed into shell history. The paging `continuation` argument is an exception: a `null` token means the result set is exhausted, so passing it back is rejected. The `rm` safety options `partition-key` (including alias `pk`) and `etag` also reject explicit `null` values rather than silently dropping the partition scope or concurrency check. +Explicit `null` argument values are treated as omitted and are not echoed into shell history. The paging `continuation` argument is an exception: a `null` output token means the result set is exhausted unless `resultIncomplete` is `true`, in which case results were truncated and cannot be resumed. A `null` token must never be passed back as `continuation`; doing so is rejected in either case. For incomplete results, retry with a larger `max` or a narrower query. The `rm` safety options `partition-key` (including alias `pk`) and `etag` also reject explicit `null` values rather than silently dropping the partition scope or concurrency check. Your MCP client may use a remote LLM. Command outputs, query results, and file contents could be transmitted to external services. **Treat all shell output as potentially shared.**