diff --git a/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs b/CosmosDBShell.Tests/ToolOperationsCallToolTests.cs index a83ffafb..1e06c319 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; @@ -233,6 +234,35 @@ public async Task CallTool_RmWithJsonNullSafetyOption_ConfirmedCommandFailsWitho Assert.IsType(Assert.Single(result.Content)).Text); } + [Theory] + [InlineData("etag", "command-rm-error-etag_empty")] + [InlineData("partition-key", "command-rm-error-partition_key_missing_value")] + [InlineData("pk", "command-rm-error-partition_key_missing_value")] + public async Task CallTool_NullRmSafetyOption_ReturnsErrorBeforeConfirmation(string argumentName, string errorKey) + { + var history = ShellInterpreter.Instance.History.ToArray(); + var arguments = new Dictionary + { + ["pattern"] = Json("\"order-123\""), + ["key"] = Json("\"id\""), + [argumentName] = Json("null"), + }; + + var result = await CreateToolOperations().CallToolHandler( + CallContext("rm", arguments), TestContext.Current.CancellationToken); + + var (isError, root, document) = ReadResult(result); + using (document) + { + Assert.True(isError); + Assert.Equal( + Azure.Data.Cosmos.Shell.Util.MessageService.GetString(errorKey), + root.GetProperty("error").GetString()); + } + + Assert.Equal(history, ShellInterpreter.Instance.History); + } + [Fact] public async Task ConfirmDestructive_UserAccepts_ReturnsNull() { @@ -445,6 +475,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() { 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 2f0f47df..5389c86a 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; @@ -31,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; @@ -234,7 +235,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. @@ -271,18 +272,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) @@ -434,6 +440,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, @@ -454,7 +465,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) { @@ -554,6 +565,26 @@ private async ValueTask OnCallToolsAsync( var option = command.Options.FirstOrDefault(a => MatchesArgumentName(a.Name, par.Key)); if (option != null) { + if (IsExplicitJsonNull(par.Value)) + { + var safetyErrorKey = cmd is RmCommand + ? option.PropertyInfo.Name switch + { + nameof(RmCommand.ETag) => "command-rm-error-etag_empty", + nameof(RmCommand.PartitionKeyArgument) => "command-rm-error-partition_key_missing_value", + _ => null, + } + : null; + if (safetyErrorKey != null) + { + var errorMessage = MessageService.GetString(safetyErrorKey); + this.logger?.LogWarning("{Message}", errorMessage); + return McpResponseFactory.CreateError(errorMessage, ShellInterpreter.Instance.State); + } + + continue; + } + var bindError = this.BindMember( cmd, option.PropertyInfo, @@ -573,6 +604,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/README.md b/README.md index 650805c9..d25bce44 100644 --- a/README.md +++ b/README.md @@ -37,7 +37,7 @@ A terminal-native shell for Azure Cosmos DB — navigate databases like a filesy Exports replace their destination only after successful completion, preserving an existing file on failure or cancellation. Imports stream records; CSV exports use temporary disk storage to discover columns without retaining all documents in memory. Scalar CSV results use an empty column header by default; set `COSMOSDB_SHELL_CSV_SCALAR_COLUMN` to name it. An import rejects populated columns with empty headers rather than discarding their values. See [import/export](docs/commands.md#export). -MCP command execution is serialized with the shell, and destructive confirmations are invalidated by connection or navigation changes. MCP invocations are echoed in the shell so their activity stays visible, and they are recorded in history alongside interactive commands. Concurrent shells merge history under a shared lock and publish complete replacements instead of truncating the saved file. History remains fully replayable, including connection strings; treat its file as sensitive. See [MCP security](docs/mcp.md#security) and [history](docs/navigation.md#history). +MCP command execution is serialized with the shell, and destructive confirmations are invalidated by connection or navigation changes. Ordinary explicit null MCP arguments are omitted; null continuation tokens and null `rm` partition-key/ETag safety options are rejected. MCP invocations are echoed in the shell so their activity stays visible, and they are recorded in history alongside interactive commands. Concurrent shells merge history under a shared lock and publish complete replacements instead of truncating the saved file. History remains fully replayable, including connection strings; treat its file as sensitive. See [MCP security](docs/mcp.md#security) and [history](docs/navigation.md#history). MCP clients supporting resource subscriptions can watch `cosmos://shell/current-location` for interactive navigation and connection changes; the resource includes the current account endpoint separately from the location. See [MCP location updates](docs/mcp.md#shell-location-updates). diff --git a/docs/mcp.md b/docs/mcp.md index 28e70983..a5c7d9fc 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -114,6 +114,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 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.** ### Best Practices