Files
Joseph Doherty f4919ba97c
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m19s
ci / java (push) Successful in 2m25s
ci / portable (push) Successful in 8m21s
docs(archreview): cycle-3 architecture review (2026-08-19 @ 8b3e3b3)
Six domain reports + overall synthesis. All 47 cycle-2 Done claims verified
at HEAD with zero false Dones and zero regressions; 34 new findings
(0 Critical, 0 High, 4 Medium, 25 Low, 5 Info): GWC-31..38, WRK-32..34,
IPC-33..39, SEC-37..41, CLI-46..50, TST-31..36.
2026-08-20 09:10:12 -04:00

24 KiB
Raw Permalink Blame History

Language Clients — Architecture Review (Cycle 3)

Date: 2026-08-19 · Commit: 8b3e3b3 (main) · Baseline: 4f5371f (2026-07-12 cycle) · Method: static review on macOS (no builds, no tests run)

Scope: clients/dotnet (lib + CLI + tests), clients/go (mxgw-go), clients/java, clients/python, clients/rust (lib + CLI + build.rs + vendored protos), cross-client parity, docs/CrossLanguageSmokeMatrix.md, docs/ClientLibrariesDesign.md, docs/ClientPackaging.md, docs/ClientBehaviorFixtures.md, client READMEs, scripts/pack-clients.ps1 / scripts/tag-go-module.ps1. Prior findings tracked as CLI-01..34 (archreview/remediation/00-tracking.md) and CLI-35..45 (archreview/2026-07-12/remediation/50-clients.md + 00-tracking.md), plus NEXT-02 (archreview/2026-07-12/remediation/90-candidate-findings-next-cycle.md).

Since 4f5371f the client family absorbed: the full CLI-35..45 remediation train, NEXT-02, the 0.2.0/0.2.1 family publish (2026-08-07, 5b153da), TST-24 real-wire tests for .NET and Python (a8f86b5), alarm truncation-flag regen (d9ea8a8), the feed-level snapshot_status rollout (c748361 + 4d4f443), StreamAlarms open-sequence doc truth (c3c603f), and Gradle-9 survival for checkGeneratedClean (df45cb4).

Prior-finding verification

2026-07-12 cycle (CLI-35..45) and NEXT-02

ID Claimed status Verified? Evidence (file:line at HEAD) Notes
CLI-35 Done Yes clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:24 (imports ReplayGap), :1660-1666 (_event_row branches on isinstance(item, ReplayGap) and emits {"replayGap": {"requestedAfterSequence": …, "oldestAvailableSequence": …}}); regression clients/python/tests/test_cli.py:935 (test_stream_events_renders_replay_gap, asserts exit 0, the gap row at index 0, the normal event row after, :971-974) Exactly the designed fix, camelCase row matching the cross-CLI shape.
CLI-36 Done Yes clients/go/cmd/mxgw-go/main.go:1012-1029 (if result.IsReplayGap() → JSON replayGapCursors struct :70, text REPLAY_GAP requested_after=%d oldest_available=%d); test clients/go/cmd/mxgw-go/main_test.go:734 (TestRunStreamEventsPrintsReplayGap over a real fake gateway :698-713, both output modes) Gap counted toward -limit as designed.
NEXT-02 Resolved 2026-08-10 Yes .NET clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:1425-1429 (stream loop branch), :1845-1846 (EventToJsonElement inside --json aggregate), :1858-1873 (FormatReplayGapRow — hand-built so cursors are JSON numbers, not proto-JSON uint64 strings); Java clients/java/zb-mom-ww-mxgateway-cli/.../MxGatewayCli.java:1665-1674 (replayGap JSON row + REPLAY_GAP text line); tests MxGatewayCliTests.java:909-937, clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientCliTests.cs (grep FormatReplayGapRow/replayGap hits); docs/CrossLanguageSmokeMatrix.md:43-67 now records one converged contract with a per-CLI table All five CLIs emit the same two camelCase keys with numeric cursors; the matrix doc explains why the row is hand-typed (proto3 JSON stringifies uint64). Genuinely converged.
CLI-37 Done Yes — all five conformant on category .NET MxStatusProxyExtensions.cs:15-20 (Category is MxStatusCategory.Ok only); Go clients/go/mxgateway/status.go:15-17 (nil ∥ category == OK, doc :7-14 states the diagnostics-only rule); Java MxStatuses.java:33-35 (+ success() Javadoc :55-57 marks it diagnostic); Python errors.py:144-152; Rust error.rs:407-413 (status.category != Ok as i32, doc :383-388 covers the unspecified-category-fails edge); shared fixtures clients/proto/fixtures/behavior/command-replies/write.status-category-error-success-set.reply.json + write.status-category-ok-success-zero.reply.json, registered in the manifest, documented in docs/ClientBehaviorFixtures.md (Reply Validation Conformance table), and consumed by all five suites (MxCommandReplyExtensionsTests.cs, protofixtures_test.go, MxGatewayFixtureTests.java, test_errors.py, client_behavior.rs) The 4-vs-1 divergence is gone; the edge decisions (absent entry = success, present-unspecified = failure) are uniform and documented.
CLI-38 Done Yes — all five on hresult < 0 .NET MxCommandReplyExtensions.cs:38 (HasHresult && Hresult < 0); Go errors.go:299 (GetHresult() < 0); Java MxGatewayErrors.java:52 (getHresult() < 0); Python errors.py:137; Rust error.rs:407; fixtures write.hresult-s-false.reply.json (S_FALSE=1 → success) + write.hresult-e-fail.reply.json (E_FAIL → failure) wired into all five suites Lands prior-cycle CLI-08 (cycle-1 tracker row updated to Done via CLI-38 — verified). docs/ClientLibrariesDesign.md:112-125 is now true, and states both rules explicitly.
CLI-39 Done Yes (bump + both guards); pattern has already recurred → CLI-46 Versions: clients/rust/Cargo.toml:3/:28 = 0.2.0, clients/python/pyproject.toml:9 + version.py:3 = 0.2.0 (+ new drift test test_cli.py:17 test_version_matches_pyproject_toml, closing the CLI-26 residual), clients/go/mxgateway/version.go = 0.2.0, .../ZB.MOM.WW.MxGateway.Client.csproj:22 = 0.2.0, Contracts .csproj:10 = 0.2.0 (440e7cf), Java build.gradle = 0.2.1. Guards: scripts/tag-go-module.ps1:40-53 (tag fails on ClientVersion mismatch — the CLI-21 guard finally implemented), scripts/pack-clients.ps1:91-156 (Test-GiteaPackageExists + hard abort "never force-overwrites"). Publish record: docs/ClientPackaging.md:102-113 (2026-08-07: nuget/pypi/cargo 0.2.0, maven 0.2.1 with the rationale for the Java exception, Go tag clients/go/v0.2.0 — tag exists in this repo) Executed to the letter, including the doc's "bump before every publish" rule and the collision-guard rationale. However the same hazard is live again at HEAD: post-publish library changes (incl. a shipped Python connect() fix) sit at the already-published 0.2.0/0.2.1 — see CLI-46.
CLI-40 Done Yes Java MxGatewaySecrets.java:62-78 (redactExact); .NET MxGatewaySecretRedaction.cs (Redact/RedactReply:55-68/RedactStatus:81-89/RedactStatuses — scrubs message and the structured reply accessors, the 0d874f9 follow-up); Rust error.rs:208-231 (MxAccessError.secrets + with_secrets, hand-written Debug prints only the count :202-204); fixtures authenticate-user.echoed-credential.reply.json + .echoed-credential-mxaccess-failure.reply.json in the manifest, consumed by all five suites (MxGatewaySessionReplyContractTests.cs, command_reply_fixtures_test.go, MxGatewayCredentialReplyTests.java, test_malformed_reply.py/test_errors.py, client_behavior.rs) The echoed-credential fixture is exactly the test the prior review said was missing ("the fakes don't echo").
CLI-41 Done Yes Go errors.go:164-176 (MalformedReplyError); Python errors.py:56; Java MxGatewayMalformedReplyException.java + throw sites MxGatewaySession.java:889/:917/:948 (redaction rethrow preserves the type :1143-1144); .NET MxGatewayMalformedReplyException.cs; Rust session.rs:1024-1042 (payload → present return_value → Error::MalformedReply, now uniform across its extractors); fixtures authenticate-user.missing-payload.reply.json + authenticate-user.return-value-only.reply.json; contract documented in docs/ClientLibrariesDesign.md ("Malformed-reply extraction" paragraph) Silent-0 user ids and the .NET NRE are gone; the payload → return_value → typed-error ladder is uniform.
CLI-42 Done Yes clients/rust/README.md:24-36 (repo-path-first / vendored-fallback resolution, same-commit refresh rule, both-direction drift guard); docs/ClientPackaging.md:199-213 (same, plus why cargo package runs with verification); 711793f extended check-codegen Check 3 to sweep both directions (canonical proto missing a vendored copy now fails) Vendored copies verified byte-identical to Contracts at HEAD (all three protos, diff clean).
CLI-43 Done Yes docs/style-guides/JavaStyleGuide.md:8 — "Target Java 17 (the Ignition 8.3 baseline; the client build enforces …)" grep "Java 21" in style-guides: clean.
CLI-44 Done Yes clients/go/mxgateway/session.go:1077 (Recv-error path uses sendTerminalEventResult), :1096-1111 (bypasses the overflow substitution via the reserved slot) Root cause of a genuine stream error is no longer relabeled ErrSlowConsumer.
CLI-45 Done Yes Canonical env MXGATEWAY_VERIFY_PASSWORD in all five: Go main.go:452 (default) + :483-491 (fail-fast before dialing, message names flag+var only); Java MxGatewayCli.java:194 + :1166-1179 (ParameterException on null/blank); Rust main.rs:151 (clap default) + test :2644-2660; Python commands.py:38 (DEFAULT_VERIFY_PASSWORD_ENV); .NET MxGatewayClientCli.cs:353-359 (canonical default + MXGATEWAY_VERIFY_USER_PASSWORD/--verify-user-password* kept as deprecated aliases, resolution order documented :385-386 and in help :2455-2456). Tests: Go main_test.go:605 (RejectsEmptyPassword) + :632 (ReadsPasswordFromCanonicalEnv). Contract documented at docs/CrossLanguageSmokeMatrix.md:90-119, incl. the parity rationale (libraries still transmit whatever they are given) and the .NET deprecation window One exported variable now drives all five CLIs; empty credential is a usage error everywhere.

Done claims that don't fully hold: none. Every 2026-07-12 remediation claim checked out at HEAD with the designed shape (fixtures-first conformance, docs in the same commit, guards actually implemented — including the CLI-21 tag guard skipped last cycle). This is the cleanest verification pass of the three cycles.

Cycle-1 residue (CLI-05..33, previously Open)

Re-verified briefly at HEAD; all unchanged unless noted:

  • CLI-08 → Done via CLI-38 (see above); cycle-1 tracker row correctly updated.
  • Still open, unchanged: CLI-05 (MxGatewaySession.cs:19 internal ctor, no .NET AttachSession), CLI-06 (:1501-1505 DisposeAsync → unbounded CloseAsync), CLI-07 (MxGatewayClient.cs:311), CLI-09 (no typed auth errors in Go errors.go), CLI-10 (client.go:64/:68 WithBlock/DialContext), CLI-11 (library has RequireCertificateValidation (client.go:239) but mxgw-go still exposes no flag), CLI-13 (MxGatewayClient.java:248 hardcoded 16), CLI-14 (no TOFU runtime warning in options.py), CLI-17/20 (TLS posture divergence: .NET accept-all MxGatewayClient.cs:369, Go skip-verify, Java InsecureTrustManagerFactory MxGatewayClient.java:391, Python TOFU, Rust strict), CLI-19 (duplicate InternalsVisibleTo), CLI-22 (session.go:1176-1181 empty correlation id on rand failure), CLI-23 (nil-vs-empty asymmetry — now at least documented in the WriteBulk doc comment session.go:431-432), CLI-25 (close() doesn't await termination MxGatewayClient.java:347-351), CLI-27 (session.py close still lock-free), CLI-28 (bottom-of-file import session.py), CLI-31 (mxgw-cli/src/main.rs now 2,991 lines — grew again; the other CLIs sit at 1,668–2,478), CLI-32 (no .NET/Java client-side bulk cap), CLI-33 (no backpressure section in docs/ClientBehaviorFixtures.md).

New findings

CLI-46 — Version constants again equal the already-published 0.2.0/0.2.1 while post-publish changes shipped — including the fix for a published-broken Python connect()

Severity: Medium · Category: packaging / release hygiene Files: clients/python/pyproject.toml:9 + clients/python/src/zb_mom_ww_mxgateway/version.py:3 (0.2.0 = the pypi artifact published 2026-08-07, docs/ClientPackaging.md:102-113); clients/python/src/zb_mom_ww_mxgateway/client.py:61-72 (the fixed connect()); post-publish library commits a8f86b5 (2026-08-10), d9ea8a8, c748361 (2026-08-17/18); guard scripts/pack-clients.ps1:145-156 The published pypi 0.2.0 Python client cannot open a real connection at all: its connect() built the grpc.aio channel inside asyncio.to_thread, and a grpc.aio channel binds to the event loop current on the constructing thread, so every non-stub connection raised "There is no current event loop in thread" (TST-24's first real-wire run caught it; fixed in-tree by a8f86b5 — resolve_channel_security runs off-loop, create_channel on-loop). That fix has sat unpublished for nine days while the version constant still reads 0.2.0. Since the publish, all five libraries also gained wire-surface additions at the published numbers (alarm truncation fields d9ea8a8, feed-level snapshot_status bindings c748361; Java at its published 0.2.1). This is precisely the CLI-39 pattern one cycle later — now mitigated (the pack-script collision guard refuses a same-version publish, and the bump-together deferral is deliberate) but the live registry keeps serving a broken Python client, and nothing in the tree records that 0.2.0-as-published ≠ 0.2.0-as-source. Recommendation: cut the next release train soon (0.2.1 four clients / 0.2.2 Java, or 0.3.0 across the board), prioritizing the Python publish; until then, note the known-broken connect() against pypi 0.2.0 in clients/python/README.md/docs/ClientPackaging.md (or yank the pypi artifact). Consider bumping version constants in the same commit as the first post-publish library change rather than at publish time — the collision guard then enforces nothing because nothing needs catching.

CLI-47 — snapshot-status/provider-status rendering diverges: .NET text mode hides the flag on the frames that matter most; Rust's JSON is the lone always-explicit shape

Severity: Low · Category: cross-client parity / CLI Files: .NET MxGatewayClientCli.cs:15 (JsonFormatter.Default — omits default-valued proto3 fields) + :1563-1566 (text arms format the payload through it); Go main.go:1112-1114 (snapshot-status truncated=%t); Java MxGatewayCli.java:2309-2312 (truncated=%b) + protoJson :2362-2368 (default printer); Rust main.rs:2262-2264 (summary) + :2314-2318 (hand-built JSON, always includes "truncated"); Python CLI renders generic MessageToDict The c748361 rollout achieved surface parity (all five render the new arm) but not rendering parity. The snapshot_status clearing frame — truncated = false, emitted on monitor restart, explicitly called out by the commit and docs/Grpc.md as a frame consumers must see — renders in the .NET CLI's text mode as snapshot-status {} (proto3 JSON omits the false bool), while Go/Java/Rust print truncated=false explicitly. The same applies to a healthy provider-status (degraded=false invisible in .NET text). In JSON mode the split flips: Rust's hand-built row always carries "truncated", while .NET/Go/Java/Python protobuf formatters emit {"snapshotStatus": {}} for the clearing frame. Semantically recoverable (absent = false), but the same class of divergence NEXT-02 just cleaned up for ReplayGap, where the cross-CLI row was deliberately hand-specified. Recommendation: hand-format the .NET text arms like the other three (snapshot-status truncated={bool}, provider-status mode=… degraded=… reason=…); decide one JSON shape (either Rust adopts formatter-omission or the matrix documents Rust's explicitness the way CrossLanguageSmokeMatrix.md:54-67 documents the ReplayGap row's deliberate choices).

CLI-48 — The new alarm-feed renderer arms are untested in three of the four CLIs that changed; the Go CLI has no stream-alarms test at all

Severity: Low · Category: testing coverage Files: clients/go/cmd/mxgw-go/main_test.go (test inventory :16-734 — no stream-alarms/formatAlarmFeedMessage test of any kind); .NET MxGatewayClientCliTests.cs:671-706 (stream-alarms test feeds only ActiveAlarm + SnapshotComplete); Java MxGatewayCliTests.java:653-687 (asserts active-alarm/snapshot-complete/transition arms only); Rust main.rs:2986 (the one snapshot-status render test, added by 4d4f443) The review-nits commit covered only the Rust render; the .NET/Go/Java provider-status and snapshot-status arms shipped with zero assertions. The .NET {}-rendering divergence (CLI-47) is exactly what a text-mode assertion on a truncated=false frame would have caught — cycle 2's lesson ("each new epic achieves surface parity but re-introduces behavioral divergence") repeating at the test layer. Recommendation: one fake-fed stream-alarms test per CLI whose feed includes a provider_status and a snapshot_status frame (including the false/clearing shape), asserting the rendered row in both output modes; for Go this also creates the missing stream-alarms scaffold.

CLI-49 — Go library exposes no aliases for AlarmProviderStatus/AlarmSnapshotStatus, whose generated types live in an internal package; the AlarmFeedMessage alias doc names three of five payload arms

Severity: Low · Category: code organization & conventions / documentation Files: clients/go/mxgateway/types.go:129-137 (alias block: AlarmFeedMessage, ActiveAlarmSnapshot, OnAlarmTransitionEvent — but not the two status payloads; ReplayGap got its alias at :38 when it shipped); generated types under clients/go/internal/generated (Go internal visibility) Because the generated package is internal, a Go consumer cannot name *generated.AlarmSnapshotStatus: they can call message.GetSnapshotStatus().GetTruncated() on an inferred value but cannot declare a variable, struct field, or function parameter of the type — unlike every other StreamAlarms payload, which types.go deliberately aliases for exactly this reason. (AlarmProviderStatus has had the same gap since provider-mode shipped.) The AlarmFeedMessage alias comment (types.go:130-132) still describes the oneof as "an active-alarm snapshot row, a snapshot-complete sentinel, or a transition" — stale by two arms. Recommendation: add AlarmProviderStatus = pb.AlarmProviderStatus and AlarmSnapshotStatus = pb.AlarmSnapshotStatus aliases and refresh the AlarmFeedMessage comment to name all five arms.

CLI-50 — Python CLI stream-alarms default (--max-messages 1, collect-then-print) now returns only the provider-status preamble — the README's own example fetches no alarm data

Severity: Low · Category: cross-client parity / CLI usability Files: clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:514 (--max-messages default 1), :1539-1551 (_collect_alarm_messages — bounded collect, printed once at the end), :1122-1134; clients/python/README.md:451 (mxgw-py stream-alarms --max-messages 1 --json); feed order per docs/Grpc.md/c3c603f: provider_status → snapshot_status → cached active_alarm → snapshot_complete The other four CLIs stream rows as they arrive and default to unbounded (--limit 0 / --max-events 0). Python is the outlier twice over: it buffers then prints (so it cannot tail a live feed at all), and its default of 1 — reasonable when the first frame was an alarm — now returns exactly one provider_status frame, since the feed gained a two-frame status preamble. The README example therefore demonstrates a command that yields no alarm rows. Pre-existing shape, but the preamble growth this cycle changed what the default actually returns. Recommendation: raise the default (or document that N status frames precede the data) and update the README example; longer term consider printing rows incrementally like the other four CLIs.

Severity roll-up (new findings)

Severity Count IDs
High 0 —
Medium 1 CLI-46
Low 4 CLI-47, CLI-48, CLI-49, CLI-50

Cross-client parity note (updated)

The 2026-07-12 divergence table has largely collapsed. Converged this cycle, with shared fixtures pinning the bytes: HRESULT < 0 (CLI-38), status verdict = category != OK (CLI-37), exact-secret scrub incl. echoed diagnostics (CLI-40), malformed-reply ladder (CLI-41), CLI ReplayGap row (CLI-35/36 + NEXT-02, all five, same keys, numeric cursors), credential env-var contract (CLI-45). What remains divergent:

Behavior .NET Go Java Python Rust
Version vs published artifact (CLI-46) 0.2.0 = shipped 0.2.0 = shipped 0.2.1 = shipped 0.2.0 = shipped (broken connect() on pypi) 0.2.0 = shipped
CLI text render of snapshot/provider status (CLI-47) proto-JSON, flag invisible when false explicit truncated=… explicit n/a (JSON only) explicit ✓
CLI JSON shape of snapshot_status (CLI-47) omits false omits false omits false omits false always explicit
New renderer arms tested (CLI-48) no no (no stream-alarms test) no n/a (generic dump) yes ✓
stream-alarms consumption model (CLI-50) stream, unbounded default stream, unbounded stream, unbounded collect, default 1 stream, unbounded
Session re-attach (CLI-05) No Yes Yes Yes Yes
Client-side bulk cap (CLI-32) No Yes No Yes Yes
Event backpressure (CLI-13/33) unbuffered 16+1, loud, true root cause ✓ 16, cancel+error unbuffered unbuffered
TLS default (CLI-17) accept-all accept-all accept-all TOFU pin strict pin
Typed auth errors (CLI-09) Yes No Yes Yes Yes
Real-wire test harness Kestrel h2c ✓ (TST-24) bufconn ✓ InProcessServer ✓ grpc.aio loopback ✓ (TST-24) tonic loopback ✓

CLI subcommand coverage remains uneven by design and is now honestly specified (docs/CrossLanguageSmokeMatrix.md:163-186: .NET all nine, Rust + unregister/credential pair, Go/Python/Java credential pair only) — verified accurate against all five dispatch tables at HEAD, including the note that every gap is CLI-surface-only (all five libraries carry all typed helpers, confirmed for the Rust bulk family at session.rs:605-654).

What is good

  • The remediation train executed at design fidelity. All eleven 2026-07-12 findings landed exactly as their remediation designs specified — including the parts prior cycles skipped (the CLI-21 tag guard, the CLI-02/42 doc half, the CLI-26 pyproject-drift test). The conformance findings (CLI-37/38/40/41) all went fixtures-first: ten shared command-reply fixtures under clients/proto/fixtures/behavior/command-replies/, each consumed by all five suites, so the canonical rules are pinned in bytes, not prose.
  • The publish pipeline is now guarded end to end: version-vs-tag check in tag-go-module.ps1, registry-collision hard-stop in pack-clients.ps1, publish coordinates and the Java 0.2.1 exception recorded in docs/ClientPackaging.md with the reasoning ("these are the numbers a future release bumps off").
  • TST-24 paid for itself immediately: the first real-wire run of the Python client found connect() could never work outside stubs — a defect every mock-based test had actively asserted as correct. All five clients now have genuine-transport tests (framing, metadata, status codes, ReplayGap, PERMISSION_DENIED mapping), and CI runs all five suites plus a Java generated-tree drift check (.gitea/workflows/ci.yml:84-142).
  • Generated-code hygiene is the best it has been: vendored Rust protos byte-identical to Contracts (verified, all three files); check-codegen Check 3 sweeps both directions; the Java toolchain pin eliminated the spurious ~64k-line protobuf churn (per build.gradle:61-70, IPC-24) and checkGeneratedClean survived Gradle 9 (df45cb4); Python regen stays on the pinned grpcio-tools 1.80.0 / protobuf 6.31.1 baseline (pyproject.toml:41-44, generated header confirms 6.31.1); even a proto-comment-only change (c3c603f) regenerated every downstream tree in the same commit.
  • Parity discipline held through the alarm epic: no client wraps or reinterprets AlarmFeedMessage/ActiveAlarmSnapshot — the truncation and status flags pass through the generated types untouched in all five, and every README explains the from_truncated_snapshot/snapshot_status semantics with the same don't-reconcile-deletions guidance, explicitly disambiguating them from the per-record degraded flag.
  • The smoke-matrix doc is a model of honest divergence-recording: where the CLIs still differ (subcommand coverage, JSON formatting quirks per language), the doc specifies the delta instead of leaving it to be discovered — the exact practice this review series asked for two cycles ago.