From fb2b1a4a526112b25f30a70cb5bb7feb597511c6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 15 Jun 2026 02:39:11 -0400 Subject: [PATCH] fix(client/dotnet): restore warnings-as-errors floor; license metadata; LazyBrowseNode publication (Client.Dotnet-022..025) --- clients/dotnet/Directory.Build.props | 19 +++- clients/dotnet/LICENSE.txt | 12 +++ .../FakeGalaxyRepositoryTransport.cs | 22 ++-- .../LazyBrowseNodeTests.cs | 90 ++++++++++++++++ .../LazyBrowseNode.cs | 29 ++++- .../ZB.MOM.WW.MxGateway.Client.csproj | 5 + code-reviews/Client.Dotnet/findings.md | 100 +++++++++++++++++- 7 files changed, 263 insertions(+), 14 deletions(-) create mode 100644 clients/dotnet/LICENSE.txt diff --git a/clients/dotnet/Directory.Build.props b/clients/dotnet/Directory.Build.props index 7fb9866..971cae5 100644 --- a/clients/dotnet/Directory.Build.props +++ b/clients/dotnet/Directory.Build.props @@ -1,4 +1,17 @@ + + + latest + enable + enable + true + latest + true + true + + Joseph Doherty @@ -10,11 +23,15 @@ https://gitea.dohertylan.com/dohertj2/mxaccessgw mxaccess;mxgateway;grpc;client;archestra false + + LICENSE.txt 0.1.0 true snupkg - true false diff --git a/clients/dotnet/LICENSE.txt b/clients/dotnet/LICENSE.txt new file mode 100644 index 0000000..2623317 --- /dev/null +++ b/clients/dotnet/LICENSE.txt @@ -0,0 +1,12 @@ +Proprietary License + +Copyright (c) ZB MOM WW. All rights reserved. + +This software and its source code are proprietary and confidential. They are +licensed, not sold, for internal use within ZB MOM WW and its authorized +partners only. No part of this package may be reproduced, distributed, or +transmitted to third parties without the prior written permission of ZB MOM WW. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE, AND NONINFRINGEMENT. diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/FakeGalaxyRepositoryTransport.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/FakeGalaxyRepositoryTransport.cs index 1ec00d1..a71d74c 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/FakeGalaxyRepositoryTransport.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/FakeGalaxyRepositoryTransport.cs @@ -135,25 +135,35 @@ internal sealed class FakeGalaxyRepositoryTransport(MxGatewayClientOptions optio /// Queue of exceptions to throw from BrowseChildren; dequeued in FIFO order. public Queue BrowseChildrenExceptions { get; } = new(); + /// + /// Optional hook awaited inside BrowseChildren before the reply is produced. Lets a + /// test hold an RPC mid-flight to exercise concurrent reads of the in-progress node. + /// + public Func? BrowseChildrenGate { get; set; } + /// /// Records the request and either throws a queued exception or returns the configured reply. /// /// The BrowseChildrenRequest to process. /// Call options specifying RPC behavior. - public Task BrowseChildrenAsync( + public async Task BrowseChildrenAsync( BrowseChildrenRequest request, CallOptions callOptions) { BrowseChildrenCalls.Add((request, callOptions)); if (BrowseChildrenExceptions.TryDequeue(out Exception? exception)) { - return Task.FromException(exception); + throw exception; } - return Task.FromResult( - BrowseChildrenReplies.TryDequeue(out BrowseChildrenReply? reply) - ? reply - : BrowseChildrenReply); + if (BrowseChildrenGate is { } gate) + { + await gate().ConfigureAwait(false); + } + + return BrowseChildrenReplies.TryDequeue(out BrowseChildrenReply? reply) + ? reply + : BrowseChildrenReply; } /// diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/LazyBrowseNodeTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/LazyBrowseNodeTests.cs index b4d5452..875c9fa 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/LazyBrowseNodeTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/LazyBrowseNodeTests.cs @@ -175,6 +175,96 @@ public sealed class LazyBrowseNodeTests Assert.Equal(2, transport.BrowseChildrenCalls.Count); } + /// + /// Verifies that reading Children/IsExpanded concurrently with an in-flight ExpandAsync + /// never throws (no torn enumeration of a mid-append list) and, once IsExpanded flips to + /// true, the published Children snapshot is fully populated. Pins the safe-publication + /// contract on the lock-free readers (Client.Dotnet-025). + /// + [Fact] + public async Task Expand_ConcurrentReadOfChildren_NeverTearsAndPublishesAtomically() + { + FakeGalaxyRepositoryTransport transport = CreateTransport(); + transport.BrowseChildrenReplies.Enqueue(BuildReply( + children: [BuildObject(1, "Plant", isArea: true)], + childHasChildren: [true], + cacheSequence: 1)); + + // Multi-page child set so the expand loop spends meaningful time appending, + // widening the window for a concurrent reader to observe a torn list. + BrowseChildrenReply childPage1 = BuildReply( + children: [BuildObject(10, "A"), BuildObject(11, "B"), BuildObject(12, "C")], + childHasChildren: [false, false, false], + cacheSequence: 1); + childPage1.NextPageToken = "1:p:3"; + transport.BrowseChildrenReplies.Enqueue(childPage1); + transport.BrowseChildrenReplies.Enqueue(BuildReply( + children: [BuildObject(13, "D"), BuildObject(14, "E")], + childHasChildren: [false, false], + cacheSequence: 1)); + + await using GalaxyRepositoryClient client = CreateClient(transport); + IReadOnlyList roots = await client.BrowseAsync(); + LazyBrowseNode node = roots[0]; + + // Gate the child-page RPCs so the expand stays mid-flight while the reader spins. + using SemaphoreSlim release = new(0, 1); + bool firstChildCall = true; + transport.BrowseChildrenGate = async () => + { + if (firstChildCall) + { + firstChildCall = false; + await release.WaitAsync().ConfigureAwait(false); + } + }; + + using CancellationTokenSource readerStop = new(); + Exception? readerFailure = null; + Task reader = Task.Run(() => + { + try + { + while (!readerStop.IsCancellationRequested) + { + bool expanded = node.IsExpanded; + + // Enumerate the snapshot; a torn/mid-append list would throw here. + int count = 0; + foreach (LazyBrowseNode _ in node.Children) + { + count++; + } + + // If the node reports expanded, the published snapshot must be complete. + if (expanded) + { + Assert.Equal(5, count); + } + } + } + catch (Exception ex) + { + readerFailure = ex; + } + }); + + Task expand = node.ExpandAsync(); + // Let the reader spin against the empty pre-publication snapshot for a moment. + await Task.Delay(50); + release.Release(); + await expand; + + // Let the reader observe the post-publication state, then stop it. + await Task.Delay(50); + readerStop.Cancel(); + await reader; + + Assert.Null(readerFailure); + Assert.True(node.IsExpanded); + Assert.Equal(5, node.Children.Count); + } + /// /// Verifies that BrowseChildrenOptions filter fields are forwarded to the BrowseChildren request. /// diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/LazyBrowseNode.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/LazyBrowseNode.cs index 360f4f5..f4eb18e 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/LazyBrowseNode.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/LazyBrowseNode.cs @@ -12,9 +12,14 @@ public sealed class LazyBrowseNode { private readonly GalaxyRepositoryClient _client; private readonly BrowseChildrenOptions _options; - private readonly List _children = []; private readonly SemaphoreSlim _expandLock = new(1, 1); - private bool _isExpanded; + + // Published once, under _expandLock, when expansion completes. Lock-free readers + // see either the empty pre-expansion snapshot or the fully-populated post-expansion + // snapshot — never a partially-filled list — because the snapshot is built in a local + // and handed off via Volatile.Write (release) paired with Volatile.Read (acquire). + private IReadOnlyList _children = []; + private volatile bool _isExpanded; internal LazyBrowseNode( GalaxyRepositoryClient client, @@ -35,7 +40,7 @@ public sealed class LazyBrowseNode public bool HasChildrenHint { get; } /// Direct children loaded by ; empty until then. - public IReadOnlyList Children => _children; + public IReadOnlyList Children => Volatile.Read(ref _children); /// True after the first call completes. public bool IsExpanded => _isExpanded; @@ -46,7 +51,13 @@ public sealed class LazyBrowseNode /// /// /// Thread-safe: concurrent callers see exactly one fetch; subsequent callers - /// (after the first completes) return immediately. + /// (after the first completes) return immediately. and + /// may be read concurrently with an in-flight + /// on another thread; the populated children are + /// published as an immutable snapshot under a release barrier, so a reader that + /// observes as always sees the + /// fully-populated , and a reader never enumerates a + /// partially-built list. /// /// Token to observe for cancellation. public async Task ExpandAsync(CancellationToken cancellationToken = default) @@ -64,6 +75,10 @@ public sealed class LazyBrowseNode return; } + // Accumulate into a local list, never the published field, so a lock-free + // reader can never observe a half-populated collection or enumerate a list + // that is being mutated mid-append. + List children = []; string pageToken = string.Empty; HashSet seenPageTokens = new(StringComparer.Ordinal); do @@ -79,7 +94,7 @@ public sealed class LazyBrowseNode for (int i = 0; i < reply.Children.Count; i++) { bool hint = i < reply.ChildHasChildren.Count && reply.ChildHasChildren[i]; - _children.Add(new LazyBrowseNode(_client, reply.Children[i], hint, _options)); + children.Add(new LazyBrowseNode(_client, reply.Children[i], hint, _options)); } pageToken = reply.NextPageToken; @@ -91,6 +106,10 @@ public sealed class LazyBrowseNode } while (!string.IsNullOrWhiteSpace(pageToken)); + // Publish the completed, immutable snapshot (release) before marking the node + // expanded (the volatile write below). A reader that observes IsExpanded == true + // is guaranteed to also observe the fully-populated Children. + Volatile.Write(ref _children, children); _isExpanded = true; } finally diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj index bf34ef7..4df7458 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj @@ -21,10 +21,15 @@ ZB.MOM.WW.MxGateway.Client .NET 10 gRPC client for the MxAccessGateway service. Provides typed wrappers, retry, and a lazy-browse walker over the Galaxy Repository hierarchy. README.md + + true + diff --git a/code-reviews/Client.Dotnet/findings.md b/code-reviews/Client.Dotnet/findings.md index 8dc7543..e060759 100644 --- a/code-reviews/Client.Dotnet/findings.md +++ b/code-reviews/Client.Dotnet/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `clients/dotnet` | | Reviewer | Claude Code | -| Review date | 2026-05-24 | -| Commit reviewed | `42b0037` | +| Review date | 2026-06-15 | +| Commit reviewed | `410acc9` | | Status | Re-reviewed | | Open findings | 0 | @@ -383,6 +383,40 @@ Re-review pass at `42b0037`. Diff against `d692232` consists of four commits: | 9 | Testing coverage | No new issues — `RunAsync_StreamAlarms_*`, `RunAsync_AcknowledgeAlarm_*`, and `RunAsync_Batch_*` give the new surface unit coverage. `bench-read-bulk` is the same stress-harness-not-SDK shape called out in the prior re-review and is not flagged here. | | 10 | Documentation & comments | Issue found (this review): the README examples for the two new alarm CLI subcommands cite wrong flag names and a non-existent `--session-id` (Client.Dotnet-018). The new XML docs on `StreamAlarmsAsync` / `AcknowledgeAlarmAsync` and on the bulk SDK methods are accurate and complete. | +#### 2026-06-15 re-review (commit 410acc9) + +Re-review pass at `410acc9`. The diff against `42b0037` is packaging/release metadata +(NuGet/Gitea feed), a TLS trust-posture option (`RequireCertificateValidation` + a +lenient accept-all default for the gateway's auto-generated self-signed cert), the +Galaxy `BrowseChildren` RPC plumbing plus a `LazyBrowseNode` lazy-browse walker, and +in-source resolutions of the prior pass's Client.Dotnet-018..021 (CLI flag-name README +fix, `RequireRegisterServerHandle`, `ParseTimeoutMs` negative guard, steady-state OCE +filter). The alarm-provider-fallback proto surface mentioned in the review brief is +**not** present in this diff — no `AlarmProviderMode` / `AlarmProviderStatus` / +`source_provider` / provider-mode-changed event reaches the .NET client here. + +Build is green (`dotnet build … .slnx` succeeds) and all 78 unit tests pass (1 skipped +live smoke). The build now emits **10 CS1591 warnings** that do not break the build, +because the `clients/dotnet/Directory.Build.props` enforcement floor recorded as +resolved under Client.Dotnet-012 (`TreatWarningsAsErrors` / `EnforceCodeStyleInBuild` / +`AnalysisLevel` / `Deterministic`) is **absent** from the history that reaches HEAD — +the props file at HEAD is packaging-metadata-only (Client.Dotnet-022). `git merge-base +--is-ancestor a020350 HEAD` is false: the 2026-05-20 review-sweep commit that resolved +012 is not in this line of history. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No new issues. The Galaxy `BrowseAsync` / `LazyBrowseNode.ExpandAsync` pagination correctly drains `next_page_token`, re-binds the same parent selector + filter set per page (matching the opaque-token contract), and guards against repeated tokens; the per-child `child_has_children` hint is read with an index-bounds check. The Client.Dotnet-019/021 in-source fixes (`RequireRegisterServerHandle`, `ParseTimeoutMs`) are correctly applied. | +| 2 | mxaccessgw conventions | Issue found (this review): the `clients/dotnet/Directory.Build.props` enforcement floor (warnings-as-errors / code-style enforcement) mandated by CLAUDE.md and recorded resolved under Client.Dotnet-012 is missing at HEAD; the new props file carries only packaging metadata (Client.Dotnet-022). Consumes the shared contracts project, no forked proto, `authorization: Bearer` metadata correct. | +| 3 | Concurrency & thread safety | Issue found (this review): `LazyBrowseNode.Children` and `IsExpanded` are read lock-free while `ExpandAsync` mutates `_children` and writes `_isExpanded` under `_expandLock`, with no release/acquire barrier to a concurrent reader (Client.Dotnet-025). `ExpandAsync`'s one-RPC dedup itself is correct (double-checked under the lock). | +| 4 | Error handling & resilience | No new issues — `BrowseChildrenAsync` routes `RpcException` through the shared `MapRpcException`; the bench steady-state OCE filter (Client.Dotnet-020) is correctly applied. | +| 5 | Security | No committed secret — the README Gitea-feed `dotnet nuget add source` example uses `` / `` placeholders. Note: TLS is lenient-by-default (accept-all callback when `UseTls` and no pinned CA), which disables certificate verification / MITM protection; this is an explicit, documented design choice for the gateway's auto-generated self-signed cert and is opt-out via `RequireCertificateValidation` or CA pinning, so not flagged as a finding. | +| 6 | Performance & resource management | No issues found — `LazyBrowseNode` holds one `SemaphoreSlim` per node (never disposed, but it owns no unmanaged handle and the node lifetime is the tree's); browse paging caps at 500/page. | +| 7 | Design-document adherence | No issues found — `BrowseChildren` / lazy-browse match `docs/GalaxyRepository.md#browsechildren`; the TLS posture matches `docs/GatewayConfiguration.md` (`RequireCertificateValidation` default `false`) and `DotnetClientDesign.md`. | +| 8 | Code organization & conventions | Issue found (this review): Client.Dotnet-022 (lost enforcement props); the new `GenerateDocumentationFile=true` in the shared props also applies to the Cli and Tests projects, surfacing CS1591 on `IMxGatewayCliClient` and every test class (Client.Dotnet-023); the client (and Contracts) NuGet package ships with no `` metadata despite setting `PackageRequireLicenseAcceptance=false` (Client.Dotnet-024). The nuspec correctly emits the transitive `ZB.MOM.WW.MxGateway.Contracts 0.1.0` dependency, so the README "pulled in transitively" claim holds. | +| 9 | Testing coverage | No new issues — `LazyBrowseNodeTests` (7 cases incl. multi-page, concurrent-expand-one-RPC, filter forwarding), `MxGatewayClientTlsHandlerTests` / `GalaxyRepositoryClientTlsHandlerTests`, and the README-example parse tests give the new surface good coverage. | +| 10 | Documentation & comments | No new issues — README NuGet-install / lazy-browse / TLS-trust sections are accurate, cross-doc anchors (`#automatic-self-signed-certificate`, `#browsechildren`) resolve, and the new XML docs on `BrowseAsync` / `LazyBrowseNode` / `RequireCertificateValidation` are complete. (The CS1591-surfaced missing docs are tracked under Client.Dotnet-023.) | + ### Client.Dotnet-018 | Field | Value | @@ -507,3 +541,65 @@ uint timeoutMs = (uint)timeoutMsRaw; A single shared helper (e.g. `ParseTimeoutMs(CliArguments, string, int)`) on `MxGatewayClientCli` would cover both call sites and remove the duplication. **Resolution:** 2026-05-24 — Confirmed against source: both `ReadBulkAsync` (line 490) and `BenchReadBulkAsync` (line 715) cast `arguments.GetInt32("timeout-ms", ...)` straight to `uint`, so `--timeout-ms -1` silently wrapped to `0xFFFFFFFF` (~49.7 days). Added a single shared private helper `ParseTimeoutMs(CliArguments arguments, int defaultValue)` on `MxGatewayClientCli` that reads the int32, rejects negatives with a clear `ArgumentException` ("--timeout-ms must be a non-negative integer (use 0 for the gateway default)."), and returns the safe `(uint)`. Both call sites now route through the helper. Regression test `MxGatewayClientCliTests.RunAsync_TimeoutMs_NegativeValue_RejectsWithClearError` (xUnit `[Theory]` over `read-bulk` and `bench-read-bulk`) drives the CLI with `--timeout-ms -1` and asserts the exit code is non-zero, that stderr contains "timeout-ms", and that the "non-negative" guard text is present. Verified red against the original `(uint)arguments.GetInt32(...)` casts (the bench proceeded past the timeout parse and tripped a downstream "Queue empty" error rather than the descriptive guard message) and green after the helper landed. + +### Client.Dotnet-022 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | mxaccessgw conventions | +| Location | `clients/dotnet/Directory.Build.props:1-21` | +| Status | Resolved | + +**Description:** Client.Dotnet-012 was recorded resolved (2026-05-20, commit `a020350`) by adding `clients/dotnet/Directory.Build.props` mirroring `src/Directory.Build.props` — `TreatWarningsAsErrors=true`, `EnforceCodeStyleInBuild=true`, `AnalysisLevel=latest`, `Deterministic=true`, `LangVersion=latest`, `Nullable=enable`, `ImplicitUsings=enable` — to restore the build-quality floor that `CLAUDE.md` calls a baseline for the .NET client. That enforcement props file is **not present in the line of history that reaches HEAD**: `git merge-base --is-ancestor a020350 HEAD` is false (the 2026-05-20 review-sweep commit was dropped during the `ZB.MOM.WW` rename / history rebuild). At `42b0037` the file did not exist at all (`git show 42b0037:clients/dotnet/Directory.Build.props` fails), and at HEAD commit `523f944` introduced a **new** `clients/dotnet/Directory.Build.props` that carries only NuGet packaging metadata (Authors/Company/RepositoryUrl/Version/etc.) — none of the enforcement properties. None of the three client `.csproj` files set `TreatWarningsAsErrors` or `EnforceCodeStyleInBuild` independently (they set only `TargetFramework` and `Nullable`). + +Net effect at HEAD: `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` **succeeds with 10 CS1591 warnings** instead of failing. The mandated quality gate that would turn new warnings (missing docs, analyzer findings, code-style violations) into build breaks is gone for the entire client tree. This is a regression of the previously-closed Client.Dotnet-012; recorded as a fresh finding at the new commit per the re-review process. + +**Recommendation:** Restore the enforcement properties in `clients/dotnet/Directory.Build.props` alongside the packaging metadata (they can coexist in the same ``), or add a sibling `clients/dotnet/Directory.Build.props` import. Re-run `dotnet build …slnx` and confirm 0 warnings / 0 errors (which will require closing Client.Dotnet-023 too, since the CS1591 warnings would otherwise become errors). Add a guard so the floor is not silently dropped again — e.g. assert the property is set in a small build test or CI check. + +**Resolution:** 2026-06-15 — Confirmed at HEAD: `clients/dotnet/Directory.Build.props` carried only packaging metadata; none of the three client `.csproj` files set the enforcement properties, so `dotnet build …slnx` succeeded with 10 CS1591 warnings instead of failing. Restored the enforcement floor in `clients/dotnet/Directory.Build.props` mirroring `src/Directory.Build.props` (`LangVersion=latest`, `Nullable=enable`, `ImplicitUsings=enable`, `TreatWarningsAsErrors=true`, `AnalysisLevel=latest`, `EnforceCodeStyleInBuild=true`, `Deterministic=true`) in a second `` alongside the existing packaging metadata. Resolved jointly with Client.Dotnet-023 (the CS1591 warnings would otherwise become errors under the restored `TreatWarningsAsErrors`). `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx -t:Rebuild` now reports 0 Warning(s) / 0 Error(s). + +### Client.Dotnet-023 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `clients/dotnet/Directory.Build.props:17`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/IMxGatewayCliClient.cs:6`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/*.cs` | +| Status | Resolved | + +**Description:** The new shared `clients/dotnet/Directory.Build.props` sets `GenerateDocumentationFile=true` at the directory level, so it applies to all three projects — including `ZB.MOM.WW.MxGateway.Client.Cli` and `ZB.MOM.WW.MxGateway.Client.Tests`, which are not packable and were not previously generating an XML doc file. Turning it on surfaces 10 CS1591 "missing XML comment" warnings: `IMxGatewayCliClient` (the public CLI interface, never documented at the type level — note Client.Dotnet-013's resolution claimed a type-level summary was added, but it is absent in the history reaching HEAD for the same reason as Client.Dotnet-022) plus every public xUnit test class (`GalaxyRepositoryClientTests`, `MxGatewayClientTlsHandlerTests`, `GalaxyRepositoryClientTlsHandlerTests`, and seven others). Today these are only warnings because the enforcement floor is missing (Client.Dotnet-022); once that floor is restored they become build-breaking errors. + +**Recommendation:** Scope `GenerateDocumentationFile=true` to the packable library project only (move it from the shared props into `ZB.MOM.WW.MxGateway.Client.csproj`, which is the only project that ships a `.nupkg`), or keep it directory-wide but suppress CS1591 on the non-public test/CLI assemblies (`$(NoWarn);CS1591` in those two `.csproj` files) and add the one-line type summary to `IMxGatewayCliClient`. The first option is cleaner and avoids documenting test classes. + +**Resolution:** 2026-06-15 — Confirmed via `-t:Rebuild`: the directory-wide `GenerateDocumentationFile=true` surfaced exactly 10 CS1591 warnings — `IMxGatewayCliClient` plus nine xUnit test classes (`GalaxyRepositoryClientTests`, `MxCommandReplyExtensionsTests`, `MxGatewayClientContractInfoTests`, `MxGatewayClientOptionsTests`, `MxGatewayClientTlsHandlerTests`, `GalaxyRepositoryClientTlsHandlerTests`, `MxGatewayGeneratedContractTests`, `MxStatusProxyExtensionsTests`, `MxValueExtensionsTests`); the shipped Client library itself emitted zero (its public surface was already fully documented). Took the first (cleaner) option, matching how `src/` handles this — only the packable `src/ZB.MOM.WW.MxGateway.Contracts.csproj` sets `GenerateDocumentationFile` directly. Removed `GenerateDocumentationFile=true` from the shared `clients/dotnet/Directory.Build.props` and moved it into the packable `ZB.MOM.WW.MxGateway.Client.csproj` only, so the Cli and Tests projects no longer generate doc files and CS1591 is not raised against them. No doc comments were added to test classes. With the Client.Dotnet-022 floor restored, the rebuild is clean (0 warnings / 0 errors). + +### Client.Dotnet-024 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `clients/dotnet/Directory.Build.props:12`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj:19-24` | +| Status | Resolved | + +**Description:** The client package sets `PackageRequireLicenseAcceptance=false` but declares **no license at all** — there is no `PackageLicenseExpression` and no `PackageLicenseFile` in `clients/dotnet/Directory.Build.props` or in the packable `.csproj`. Confirmed by packing: the emitted `ZB.MOM.WW.MxGateway.Client.0.1.0.nuspec` has no `` element, so the produced package carries no license metadata and a NuGet feed renders it as "License: not specified." The sibling `ZB.MOM.WW.MxGateway.Contracts` package (the transitive dependency) has the same gap. `dotnet pack` does not warn (a missing license is allowed), so the omission is silent. Setting `PackageRequireLicenseAcceptance=false` while shipping no license is internally inconsistent — that flag exists to control acceptance of a license that should be present. + +**Recommendation:** Add the intended license to `clients/dotnet/Directory.Build.props` (and to `ZB.MOM.WW.MxGateway.Contracts.csproj` for parity) — either `` with an SPDX id (e.g. a proprietary marker or the actual license) or `` pointing at a committed `LICENSE`. If the package is intentionally unlicensed/internal-only, document that explicitly rather than leaving the field blank. + +**Resolution:** 2026-06-15 — Confirmed via pack: the emitted nuspec had no `` element. Marked the package "Proprietary" consistent with the other clients' decision (Rust `license = "Proprietary"`, Python `license = { text = "Proprietary" }` + `License :: Other/Proprietary License`). A `LicenseRef-Proprietary` was tried first but the current NuGet toolset rejects `LicenseRef-*` (NU5124), which the restored `TreatWarningsAsErrors` escalates to a pack failure — so the proprietary terms ship as a committed license file instead: added `clients/dotnet/LICENSE.txt` (proprietary/internal-use terms), set `LICENSE.txt` in the shared `clients/dotnet/Directory.Build.props`, and packed it at the package root via a `` item in the packable `ZB.MOM.WW.MxGateway.Client.csproj`. `dotnet pack` now succeeds and the nuspec carries `LICENSE.txt` with `LICENSE.txt` present in the `.nupkg`. Scope was limited to Client.Dotnet per the constraints — the sibling `ZB.MOM.WW.MxGateway.Contracts` package has the same gap and is NOT touched here (it is a different module; flagging it for that module's review). + +### Client.Dotnet-025 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Concurrency & thread safety | +| Location | `clients/dotnet/ZB.MOM.WW.MxGateway.Client/LazyBrowseNode.cs:38,41,54,82,94` | +| Status | Resolved | + +**Description:** `LazyBrowseNode.ExpandAsync` is explicitly documented as thread-safe ("concurrent callers see exactly one fetch"), and its one-RPC dedup is correct: it double-checks `_isExpanded` under `_expandLock`. But the *readers* of the results are lock-free. `Children => _children` returns the live backing `List` reference, and `IsExpanded => _isExpanded` reads the plain `bool` field — neither takes `_expandLock` nor uses `Volatile`. A thread that observes `IsExpanded == true` (or simply enumerates `Children`) concurrently with the writer thread inside `ExpandAsync` has no release/acquire barrier guaranteeing it sees the fully-populated `_children` contents that were appended under the lock. On x86/x64 the bool read and the list-reference read are atomic and the practical risk is low, but the published-state visibility is not guaranteed by the memory model, and a reader enumerating `Children` while a concurrent `ExpandAsync` is mid-append can throw `InvalidOperationException` ("collection was modified"). This is inconsistent with the type's own thread-safety claim. + +**Recommendation:** Either (a) tighten the documented contract to "ExpandAsync is safe to call concurrently, but Children/IsExpanded must only be read after the awaited ExpandAsync completes (no concurrent reader/expander)", or (b) make the publication safe: write `_isExpanded` via `Volatile.Write` and read via `Volatile.Read`, and return an immutable snapshot from `Children` (e.g. assign a completed `IReadOnlyList` under the lock and expose that field) so lock-free readers never observe a partially-populated list. Option (a) is the smallest change and matches the realistic usage (UI thread expands then renders). + +**Resolution:** 2026-06-15 — Confirmed against source: `Children => _children` returned the live mutable backing `List` and `IsExpanded => _isExpanded` read a plain `bool`, while `ExpandAsync` appended to that same list under `_expandLock` with no release/acquire barrier to lock-free readers — so a concurrent reader could enumerate a mid-append list and throw `InvalidOperationException` ("collection was modified"). Applied option (b) (safe publication): `ExpandAsync` now accumulates children into a method-local `List` and, only when fully drained across all pages, publishes it via `Volatile.Write(ref _children, children)` (release) immediately before setting the now-`volatile bool _isExpanded = true`. The `_children` field is an `IReadOnlyList` read via `Volatile.Read` from the `Children` getter (acquire), so a reader that observes `IsExpanded == true` always sees the fully-populated snapshot and never enumerates a partially-built list. Updated the `ExpandAsync` `` to document the strengthened concurrent-read guarantee. Regression test `LazyBrowseNodeTests.Expand_ConcurrentReadOfChildren_NeverTearsAndPublishesAtomically` gates the child-page RPCs (via a new `FakeGalaxyRepositoryTransport.BrowseChildrenGate` hook) to hold the expand mid-flight while a background reader spins enumerating `Children` and reading `IsExpanded`, asserting no exception escapes and that once `IsExpanded` is true the published snapshot has all five children. Verified red against the pre-fix code (the reader threw `InvalidOperationException: Collection was modified` deterministically across three runs) and green after the fix.