diff --git a/clients/dotnet/MxGateway.Client.Cli/MxGatewayClientCli.cs b/clients/dotnet/MxGateway.Client.Cli/MxGatewayClientCli.cs index 2fcc66f..c07aab5 100644 --- a/clients/dotnet/MxGateway.Client.Cli/MxGatewayClientCli.cs +++ b/clients/dotnet/MxGateway.Client.Cli/MxGatewayClientCli.cs @@ -122,7 +122,10 @@ public static class MxGatewayClientCli } catch (Exception exception) when (exception is not OperationCanceledException) { - string? apiKey = arguments.GetOptional("api-key"); + // Redact the effective API key — whether it came from --api-key or from + // the (documented default) --api-key-env environment variable — so a + // transport error message that echoes the bearer token is never printed. + string? apiKey = TryResolveApiKey(arguments); string message = MxGatewayCliSecretRedactor.Redact(exception.Message, apiKey); if (arguments.HasFlag("json")) @@ -167,6 +170,27 @@ public static class MxGatewayClientCli } private static string ResolveApiKey(CliArguments arguments) + { + string? apiKey = TryResolveApiKey(arguments); + if (!string.IsNullOrWhiteSpace(apiKey)) + { + return apiKey; + } + + string apiKeyEnvironmentName = arguments.GetOptional("api-key-env") + ?? "MXGATEWAY_API_KEY"; + + throw new ArgumentException( + $"Gateway API key is required. Pass --api-key or set {apiKeyEnvironmentName}."); + } + + /// + /// Resolves the effective API key from --api-key or, failing that, the + /// environment variable named by --api-key-env (default + /// MXGATEWAY_API_KEY). Returns when no key is + /// configured; used for redaction where a missing key must not throw. + /// + private static string? TryResolveApiKey(CliArguments arguments) { string? apiKey = arguments.GetOptional("api-key"); if (!string.IsNullOrWhiteSpace(apiKey)) @@ -177,14 +201,7 @@ public static class MxGatewayClientCli string apiKeyEnvironmentName = arguments.GetOptional("api-key-env") ?? "MXGATEWAY_API_KEY"; - apiKey = Environment.GetEnvironmentVariable(apiKeyEnvironmentName); - if (!string.IsNullOrWhiteSpace(apiKey)) - { - return apiKey; - } - - throw new ArgumentException( - $"Gateway API key is required. Pass --api-key or set {apiKeyEnvironmentName}."); + return Environment.GetEnvironmentVariable(apiKeyEnvironmentName); } private static CancellationTokenSource CreateCancellation(CliArguments arguments, string command) diff --git a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientCliTests.cs b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientCliTests.cs index b950869..05b2072 100644 --- a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientCliTests.cs +++ b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientCliTests.cs @@ -106,6 +106,43 @@ public sealed class MxGatewayClientCliTests Assert.Contains("[redacted]", error.ToString()); } + /// + /// Verifies that error output redacts the API key even when it was sourced from + /// the --api-key-env environment variable rather than passed via + /// --api-key — the documented default credential path. + /// + [Fact] + public async Task RunAsync_ErrorOutput_RedactsApiKey_WhenSourcedFromEnvironmentVariable() + { + const string environmentVariableName = "MXGATEWAY_TEST_API_KEY_REDACT"; + using var output = new StringWriter(); + using var error = new StringWriter(); + + Environment.SetEnvironmentVariable(environmentVariableName, "env-secret-api-key"); + try + { + int exitCode = await MxGatewayClientCli.RunAsync( + [ + "open-session", + "--endpoint", + "http://localhost:5000", + "--api-key-env", + environmentVariableName, + ], + output, + error, + _ => throw new InvalidOperationException("boom env-secret-api-key")); + + Assert.Equal(1, exitCode); + Assert.DoesNotContain("env-secret-api-key", error.ToString()); + Assert.Contains("[redacted]", error.ToString()); + } + finally + { + Environment.SetEnvironmentVariable(environmentVariableName, null); + } + } + /// Verifies that stream-events with max-events limit stops output in non-JSON format. [Fact] public async Task RunAsync_StreamEvents_WithMaxEventsStopsNonJsonOutput() diff --git a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs index 3afa614..db222b6 100644 --- a/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs +++ b/clients/dotnet/MxGateway.Client.Tests/MxGatewayClientSessionTests.cs @@ -378,6 +378,84 @@ public sealed class MxGatewayClientSessionTests Assert.Equal(cancellation.Token, Assert.Single(transport.InvokeCalls).CallOptions.CancellationToken); } + /// + /// Verifies that a client-imposed is not + /// retried. The deadline budget is shared across the whole safe-unary operation, so + /// an immediate retry would only fail again — the call must surface the failure. + /// + [Fact] + public async Task InvokeAsync_DoesNotRetrySafeDiagnosticCommand_OnDeadlineExceeded() + { + FakeGatewayTransport transport = CreateTransport(); + transport.InvokeExceptions.Enqueue( + new RpcException(new Status(StatusCode.DeadlineExceeded, "deadline exceeded"))); + transport.AddInvokeReply(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.Ping, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + await Assert.ThrowsAsync(async () => await session.InvokeAsync( + new MxCommandRequest + { + SessionId = session.SessionId, + Command = new MxCommand { Kind = MxCommandKind.Ping, Ping = new PingCommand() }, + })); + + Assert.Single(transport.InvokeCalls); + } + + /// + /// Verifies that a successful register reply missing the typed register + /// payload throws a descriptive rather than + /// silently returning a zero server handle. + /// + [Fact] + public async Task RegisterAsync_Throws_WhenSuccessfulReplyMissingPayload() + { + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.Register, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + MxGatewayException exception = await Assert.ThrowsAsync( + async () => await session.RegisterAsync("client-name")); + + Assert.Contains("register", exception.Message, StringComparison.Ordinal); + } + + /// + /// Verifies that a successful add-item reply missing the typed add_item + /// payload throws a descriptive rather than + /// silently returning a zero item handle. + /// + [Fact] + public async Task AddItemAsync_Throws_WhenSuccessfulReplyMissingPayload() + { + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.AddItem, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + MxGatewayException exception = await Assert.ThrowsAsync( + async () => await session.AddItemAsync(1, "Area.Pump.Speed")); + + Assert.Contains("add_item", exception.Message, StringComparison.Ordinal); + } + private static MxGatewayClient CreateClient(FakeGatewayTransport transport) { return new MxGatewayClient(transport.Options, transport); diff --git a/clients/dotnet/MxGateway.Client/MxGatewayClient.cs b/clients/dotnet/MxGateway.Client/MxGatewayClient.cs index 5660b8b..f46b7ed 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayClient.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayClient.cs @@ -184,9 +184,10 @@ public sealed class MxGatewayClient : IAsyncDisposable /// /// Acknowledges an active MXAccess alarm condition through the gateway. The - /// gateway authenticates the request against the API key's invoke:alarm-ack - /// scope and forwards the acknowledge to the worker's MXAccess session; - /// the resulting is returned in the reply. + /// gateway authorizes against the API + /// key's admin scope (there is no finer-grained alarm-ack sub-scope) + /// and forwards the acknowledge to the worker's MXAccess session; the + /// resulting is returned in the reply. /// /// The acknowledge request — alarm reference, comment, operator user. /// Cancellation token for the operation. diff --git a/clients/dotnet/MxGateway.Client/MxGatewayClientContractInfo.cs b/clients/dotnet/MxGateway.Client/MxGatewayClientContractInfo.cs index 4869faa..1ab8cc3 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayClientContractInfo.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayClientContractInfo.cs @@ -7,9 +7,19 @@ namespace MxGateway.Client; /// public static class MxGatewayClientContractInfo { + /// + /// Gets the gateway gRPC protocol version compiled into this client package. + /// A client and gateway are wire-compatible only when this value matches the + /// gateway's advertised gateway protocol version. + /// public const uint GatewayProtocolVersion = GatewayContractInfo.GatewayProtocolVersion; + /// + /// Gets the worker frame protocol version compiled into this client package. + /// Exposed for diagnostics so callers can report the worker protocol the + /// shared contracts were generated against. + /// public const uint WorkerProtocolVersion = GatewayContractInfo.WorkerProtocolVersion; } diff --git a/clients/dotnet/MxGateway.Client/MxGatewayClientOptions.cs b/clients/dotnet/MxGateway.Client/MxGatewayClientOptions.cs index bb900c9..0840301 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayClientOptions.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayClientOptions.cs @@ -38,7 +38,12 @@ public sealed class MxGatewayClientOptions public TimeSpan ConnectTimeout { get; init; } = TimeSpan.FromSeconds(10); /// - /// Gets the default timeout for unary gRPC calls. + /// Gets the timeout budget for a unary gRPC operation. This is both the gRPC + /// deadline stamped on each individual attempt and the overall budget for the + /// whole safe-unary operation: for retryable calls the initial attempt, every + /// retry, and the backoff delays between them all share this single budget. + /// It is therefore an upper bound on the total wall-clock time a safe-unary + /// call can take, not a fresh per-retry allowance. /// public TimeSpan DefaultCallTimeout { get; init; } = TimeSpan.FromSeconds(30); @@ -47,6 +52,11 @@ public sealed class MxGatewayClientOptions /// public TimeSpan? StreamTimeout { get; init; } + /// + /// Gets the maximum size, in bytes, of a single gRPC message the client will + /// send or receive. Applied to both the send and receive limits of the + /// underlying channel. Defaults to 16 MiB. + /// public int MaxGrpcMessageBytes { get; init; } = 16 * 1024 * 1024; /// diff --git a/clients/dotnet/MxGateway.Client/MxGatewayClientRetryPolicy.cs b/clients/dotnet/MxGateway.Client/MxGatewayClientRetryPolicy.cs index af6e63d..a4a9e08 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewayClientRetryPolicy.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewayClientRetryPolicy.cs @@ -61,8 +61,13 @@ internal static class MxGatewayClientRetryPolicy private static bool IsTransientStatus(StatusCode statusCode) { + // DeadlineExceeded is intentionally NOT treated as transient. The deadline + // on every unary call is client-imposed (CreateCallOptions stamps the + // DefaultCallTimeout budget), and that same budget is shared across the + // initial attempt plus all retries plus backoff. A DeadlineExceeded means + // the shared budget is exhausted, so an immediate retry would only fail + // again — burning the remaining budget on a call that cannot succeed. return statusCode is StatusCode.Unavailable - or StatusCode.DeadlineExceeded or StatusCode.ResourceExhausted; } } diff --git a/clients/dotnet/MxGateway.Client/MxGatewaySession.cs b/clients/dotnet/MxGateway.Client/MxGatewaySession.cs index ffc023e..9876071 100644 --- a/clients/dotnet/MxGateway.Client/MxGatewaySession.cs +++ b/clients/dotnet/MxGateway.Client/MxGatewaySession.cs @@ -101,7 +101,8 @@ public sealed class MxGatewaySession : IAsyncDisposable MxCommandReply reply = await RegisterRawAsync(clientName, cancellationToken) .ConfigureAwait(false); reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.Register?.ServerHandle ?? reply.ReturnValue.Int32Value; + return reply.Register?.ServerHandle + ?? throw CreateMissingPayloadException(reply, "register"); } /// @@ -143,7 +144,8 @@ public sealed class MxGatewaySession : IAsyncDisposable cancellationToken) .ConfigureAwait(false); reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.AddItem?.ItemHandle ?? reply.ReturnValue.Int32Value; + return reply.AddItem?.ItemHandle + ?? throw CreateMissingPayloadException(reply, "add_item"); } /// @@ -194,7 +196,8 @@ public sealed class MxGatewaySession : IAsyncDisposable cancellationToken) .ConfigureAwait(false); reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.AddItem2?.ItemHandle ?? reply.ReturnValue.Int32Value; + return reply.AddItem2?.ItemHandle + ?? throw CreateMissingPayloadException(reply, "add_item2"); } /// @@ -723,4 +726,21 @@ public sealed class MxGatewaySession : IAsyncDisposable cancellationToken); } + /// + /// Builds the exception thrown when a command reply passed protocol and + /// MXAccess success checks but is missing the typed handle-bearing payload + /// the command contract requires. Surfacing this as a clear error avoids + /// silently handing a zero handle to the caller (it would otherwise fall + /// through to , which is 0 when the + /// reply carries no return value). + /// + private static MxGatewayException CreateMissingPayloadException( + MxCommandReply reply, + string expectedPayload) + { + return new MxGatewayException( + $"Gateway reply for command kind={reply.Kind} reported success but is missing " + + $"the required '{expectedPayload}' payload; cannot resolve a handle. " + + $"session={reply.SessionId}; correlation={reply.CorrelationId}"); + } } diff --git a/clients/dotnet/README.md b/clients/dotnet/README.md index 7cb9977..c4183f7 100644 --- a/clients/dotnet/README.md +++ b/clients/dotnet/README.md @@ -142,7 +142,8 @@ dotnet run --project clients/dotnet/MxGateway.Client.Cli -- smoke --endpoint htt `smoke` opens a session, registers a client, adds one item, advises it, optionally writes a value when `--type` and `--value` are supplied, reads a bounded event stream, and closes the session in a `finally` block. CLI error -output redacts API keys supplied through `--api-key`. +output redacts the effective API key, whether it was supplied through +`--api-key` or resolved from the `--api-key-env` environment variable. ## Galaxy Repository Browse diff --git a/code-reviews/Client.Dotnet/findings.md b/code-reviews/Client.Dotnet/findings.md index b2ba5fe..7cc9ddc 100644 --- a/code-reviews/Client.Dotnet/findings.md +++ b/code-reviews/Client.Dotnet/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `3cc53a8` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -78,13 +78,13 @@ | Severity | Low | | Category | Error handling & resilience | | Location | `clients/dotnet/MxGateway.Client/MxGatewayClient.cs:283-294`, `clients/dotnet/MxGateway.Client/GalaxyRepositoryClient.cs:392-403` | -| Status | Open | +| Status | Resolved | **Description:** `ExecuteSafeUnaryAsync` wraps the whole Polly retry pipeline in a single linked CTS cancelled after `Options.DefaultCallTimeout`, while `CreateCallOptions` also stamps each individual call with a `DefaultCallTimeout` gRPC deadline. The retry pipeline therefore shares one `DefaultCallTimeout` budget across the initial attempt plus all retries plus backoff delays. The README/XML docs describe `DefaultCallTimeout` as a per-call timeout, which misrepresents this. `DeadlineExceeded` is also classified as transient, so an attempt that exhausts the shared budget is retried only to immediately fail again. **Recommendation:** Decide whether `DefaultCallTimeout` is per-attempt or per-operation and make code and docs consistent — e.g. a separate per-attempt deadline and a distinct overall-operation timeout. Reconsider retrying on `DeadlineExceeded` when the deadline was client-imposed. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: the shared linked-CTS budget plus per-call deadline both use `DefaultCallTimeout`, and `IsTransientStatus` listed `DeadlineExceeded`. Resolved as a per-operation budget (the simpler, non-breaking choice): the `DefaultCallTimeout` XML doc in `MxGatewayClientOptions.cs` now states it is both the per-attempt gRPC deadline and the overall budget shared across the initial attempt, every retry, and the backoff delays — an upper bound on total wall-clock time, not a fresh per-retry allowance. Removed `DeadlineExceeded` from `MxGatewayClientRetryPolicy.IsTransientStatus`: every unary deadline is client-imposed (`CreateCallOptions` stamps the shared budget), so a `DeadlineExceeded` means the budget is exhausted and an immediate retry can only fail again. Regression test `MxGatewayClientSessionTests.InvokeAsync_DoesNotRetrySafeDiagnosticCommand_OnDeadlineExceeded` asserts the safe diagnostic command (`Ping`) is attempted exactly once and the failure surfaces; verified red against the original transient set (the call retried and succeeded). ### Client.Dotnet-005 @@ -93,13 +93,13 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `clients/dotnet/MxGateway.Client/MxGatewaySession.cs:82,124,175` | -| Status | Open | +| Status | Resolved | **Description:** `RegisterAsync`/`AddItemAsync`/`AddItem2Async` return `reply.?.ServerHandle ?? reply.ReturnValue.Int32Value`. After `EnsureMxAccessSuccess()` passes, a missing typed payload silently falls back to `ReturnValue.Int32Value`, which for a reply carrying no return value is `0`. A caller then uses `0` as a `ServerHandle`/`ItemHandle`, producing a confusing downstream invalid-handle failure rather than a clear "gateway reply missing payload" error. **Recommendation:** If the typed sub-message is the contract for these commands, treat its absence on an otherwise-successful reply as an error (throw a descriptive `MxGatewayException`) rather than falling through to `ReturnValue.Int32Value`. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source and `mxaccess_gateway.proto`: `register`/`add_item`/`add_item2` are members of the `MxCommandReply.payload` oneof, so the typed accessor is `null` whenever the worker did not set that case — and the fallback returned `ReturnValue.Int32Value` (0 for a reply with no return value). The typed sub-message is the contract for these handle-returning commands, so its absence on an otherwise-successful reply is now an error: `RegisterAsync`/`AddItemAsync`/`AddItem2Async` throw via a new private `MxGatewaySession.CreateMissingPayloadException` helper that builds a descriptive `MxGatewayException` naming the missing payload, kind, session, and correlation id. Regression tests `MxGatewayClientSessionTests.RegisterAsync_Throws_WhenSuccessfulReplyMissingPayload` and `AddItemAsync_Throws_WhenSuccessfulReplyMissingPayload` enqueue an `Ok` reply with no typed payload and assert the descriptive throw; verified red against the original fallback (returned `0` instead of throwing). ### Client.Dotnet-006 @@ -108,13 +108,13 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `clients/dotnet/MxGateway.Client/MxGatewayClientOptions.cs:50`, `clients/dotnet/MxGateway.Client/MxGatewayClientContractInfo.cs:10-14` | -| Status | Open | +| Status | Resolved | **Description:** `MxGatewayClientOptions.MaxGrpcMessageBytes` and the two `const`s in `MxGatewayClientContractInfo` are public members with no XML doc comments, inconsistent with every other public member in the assembly and with the repo's documented C# style emphasis on a documented public surface. **Recommendation:** Add `` doc comments to `MaxGrpcMessageBytes`, `GatewayProtocolVersion`, and `WorkerProtocolVersion`. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed: all three public members lacked XML docs while every other public member in the assembly is documented. Added `` comments to `MxGatewayClientOptions.MaxGrpcMessageBytes` (describing the 16 MiB default applied to both send and receive limits), and to `MxGatewayClientContractInfo.GatewayProtocolVersion` and `WorkerProtocolVersion` (describing their wire-compatibility / diagnostics purpose). Pure documentation change — no test needed; build remains warning-clean. ### Client.Dotnet-007 @@ -123,13 +123,13 @@ | Severity | Low | | Category | Documentation & comments | | Location | `clients/dotnet/MxGateway.Client/MxGatewayClient.cs:185-192` | -| Status | Open | +| Status | Resolved | **Description:** The `AcknowledgeAlarmAsync` XML comment states the gateway authenticates against an `invoke:alarm-ack` scope, but `CLAUDE.md` documents the scope set without any `invoke:alarm-ack` sub-scope. The comment may describe an intended finer-grained scope that does not exist, misleading integrators about what API key they need. **Recommendation:** Reconcile the comment with the actual server-side scope check, or update the scope documentation if sub-scopes were genuinely added; keep client doc and gateway auth model in sync. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against the server-side authorization model: `GatewayGrpcScopeResolver.ResolveRequiredScope` has no arm for `AcknowledgeAlarmRequest`, so it falls to the `_ => GatewayScopes.Admin` default — the RPC actually requires the `admin` scope. No `invoke:alarm-ack` sub-scope exists anywhere in `GatewayScopes`. The client XML comment on `AcknowledgeAlarmAsync` was wrong, not the docs. Corrected the comment to state the gateway authorizes `AcknowledgeAlarmRequest` against the API key's `admin` scope and that there is no finer-grained alarm-ack sub-scope. Pure documentation change — no test needed. ### Client.Dotnet-008 @@ -138,10 +138,10 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `clients/dotnet/MxGateway.Client.Cli/MxGatewayCliSecretRedactor.cs:9-17` | -| Status | Open | +| Status | Resolved | **Description:** The CLI redactor only removes the API key string when it was supplied via `--api-key`; `RunCoreAsync` passes `arguments.GetOptional("api-key")` to `Redact`. When the key comes from an environment variable (`--api-key-env`, the documented default path), `apiKey` is `null` and no redaction occurs. If a gRPC/transport error message ever echoes the bearer token, it would be printed unredacted. **Recommendation:** Resolve the effective API key (same logic as `ResolveApiKey`) before redacting, so the env-var-sourced key is also stripped from error output. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: `MxGatewayClientCli.RunCoreAsync`'s catch block redacted only `arguments.GetOptional("api-key")`, so an env-var-sourced key (`--api-key-env`, default `MXGATEWAY_API_KEY`) was never stripped. Note `MxGatewayCliSecretRedactor` itself is correct — the defect was the caller passing the wrong value. Extracted a non-throwing `TryResolveApiKey` helper (used by both the existing `ResolveApiKey` and the catch block) that resolves `--api-key` then the `--api-key-env` environment variable; the catch block now redacts that effective key. Updated `clients/dotnet/README.md` (`smoke` paragraph) to state the CLI redacts the effective key whether from `--api-key` or `--api-key-env`. Regression test `MxGatewayClientCliTests.RunAsync_ErrorOutput_RedactsApiKey_WhenSourcedFromEnvironmentVariable` sets a test env var, forces a transport error echoing the key, and asserts the key is absent and `[redacted]` is present; verified red against the original `GetOptional("api-key")`-only redaction (key printed unredacted).