1a5a12a416
Six-domain re-review at the P2 merge (4f5371f): all prior Done claims
verified (none false, 10 partial), 47 new findings (1 High, 14 Medium),
with per-finding design/implementation plans and a new tracking register.
252 lines
46 KiB
Markdown
252 lines
46 KiB
Markdown
# Language Clients — Remediation Design & Implementation (2026-07-12 cycle)
|
|
|
|
Source review: [50-clients.md](../50-clients.md) · Generated: 2026-07-13 · Baseline: `main @ 4f5371f`
|
|
|
|
This document turns the 2026-07-12 Language Clients re-review's **new** findings (CLI-35..CLI-45) into buildable remediation entries. Tier assignments come from the [overall roadmap](../00-overall.md). Prior-cycle findings CLI-01..CLI-34 remain tracked in `archreview/remediation/50-clients.md`; the one prior finding this cycle re-activates is **CLI-08**, which is closed by CLI-38 below.
|
|
|
|
Operating constraints carried from prior work:
|
|
|
|
- **Java builds locally now**: `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` from `clients/java`. The Gradle build regenerates the tracked `clients/java/**/src/main/generated/**/MxaccessGateway.java` with spurious protobuf-version churn (~64k lines) — **revert it (`git checkout -- clients/java/**/generated`) after every Java build in this plan, because no `.proto` changes here**.
|
|
- **No `.proto` changes anywhere in this plan** — no contracts regen, no Python `grpcio-tools` regen, no vendored-Rust-proto refresh.
|
|
- Per-language verification follows CLAUDE.md's Source Update Workflow table: `gofmt` + `go build ./...` + `go test ./...`; `cargo fmt` + `cargo check --workspace` + `cargo test --workspace` + `cargo clippy --all-targets -- -D warnings`; `python -m pytest`; `gradle test`; `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` + tests.
|
|
- **Cross-language behavior findings (CLI-37/38/41/45) are conformance exercises, not five independent fixes**: each Design section below defines one canonical behavior first (citing the wire contract), then per-language implementation. Where the behavior is specified in `docs/CrossLanguageSmokeMatrix.md` or `docs/ClientLibrariesDesign.md`, those docs change in the same commit (CLAUDE.md same-commit rule).
|
|
- Shared behavior fixtures live under `clients/proto/fixtures/behavior/` (manifest: `clients/proto/fixtures/behavior/manifest.json`, described by `docs/ClientBehaviorFixtures.md`) — new conformance cases land there so all five suites lock in the same bytes.
|
|
|
|
## Finding register
|
|
|
|
| ID | Sev | Tier | Eff | Dep | Status | Title |
|
|
|----|-----|------|-----|-----|--------|-------|
|
|
| CLI-35 | Medium | P0 | S | — | Not started | Python CLI `stream-events` crashes on a ReplayGap |
|
|
| CLI-36 | Medium | P0 | S | — | Not started | Go CLI `stream-events` silently destroys the ReplayGap signal |
|
|
| CLI-37 | Medium | P1 | M | CLI-38 | Not started | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) |
|
|
| CLI-38 | Medium | P1 | S | — | Not started | Align .NET/Go/Java on `hresult < 0` — lands prior CLI-08 and cures the design-doc drift |
|
|
| CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 | Not started | Bump client versions off the already-published 0.1.2 before the next publish; add registry-collision guard |
|
|
| CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET |
|
|
| CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem |
|
|
| CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) |
|
|
| CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" |
|
|
| CLI-44 | Low | — | S | — | Not started | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` |
|
|
| CLI-45 | Low | P1 | M | — | Not started | Standardize CLI credential env-var name and fail fast on missing/empty passwords |
|
|
|
|
Cross-domain dependencies: **CLI-35/CLI-36 pair with GWC-25** (gateway emits `oldest_available_sequence = 0` on an empty replay ring — the server-side half of the same reconnect story; the CLI fixes here are independently landable but the end-to-end resume walk in the smoke matrix needs both). **CLI-39 pairs with the publishing process** (`scripts/pack-clients.ps1`, `scripts/tag-go-module.ps1`, Gitea package registry).
|
|
|
|
---
|
|
|
|
## CLI-35 — Python CLI `stream-events` crashes on a ReplayGap `Medium` · `P0`
|
|
|
|
**Finding.** `Session.stream_events()` yields `pb.MxEvent | ReplayGap` since the CLI-15 library work (`clients/python/src/zb_mom_ww_mxgateway/session.py:816-870`; `ReplayGap` dataclass at `clients/python/src/zb_mom_ww_mxgateway/events.py:11`), but the CLI's `_stream_events` (`clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:1098-1106`) feeds every collected item into `_message_dict` (`commands.py:1627-1632`), which calls `google.protobuf.json_format.MessageToDict`. `ReplayGap` is a plain dataclass, not a proto message, so `MessageToDict` raises and the command exits with an error — after having already consumed the stream. No test covers it (`grep -i replay clients/python/tests/test_cli.py` → nothing).
|
|
|
|
**Impact.** The operator tool crashes on exactly the resume-after-outage path where it is most needed. Detach-grace and replay retention are on by default, so any `mxgw-py stream-events --after-worker-sequence` resume that outlived the replay ring hits this. P0 per the roadmap (item 1, alongside GWC-25).
|
|
|
|
**Design.** Branch on `isinstance(item, ReplayGap)` in the CLI's row formatter and emit the same distinct JSON row the Rust CLI already prints (`clients/rust/crates/mxgw-cli/src/main.rs:1019-1035`): `{"replayGap": {"requestedAfterSequence": <n>, "oldestAvailableSequence": <n>}}` — camelCase, matching `_message_dict`'s `preserving_proto_field_name=False` output for normal events, so the JSON stream stays uniform and the cross-language e2e matrix can compare rows across CLIs. The gap is rendered, never dropped and never re-synthesized into an event (no-synthesized-events invariant). Rejected alternatives: (a) yielding the raw sentinel `MxEvent` from the CLI path — would require bypassing the typed library surface the CLI already uses and reintroduces the "gap looks like a data change" hazard the library deliberately closed; (b) filtering gaps out — silent loss, precisely CLI-36's defect.
|
|
|
|
**Implementation.**
|
|
1. `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py`: import `ReplayGap` from `zb_mom_ww_mxgateway.events`. Add a small row helper, e.g. `_event_row(item)` → `_message_dict(item)` for proto messages, and for `ReplayGap` return `{"replayGap": {"requestedAfterSequence": item.requested_after_sequence, "oldestAvailableSequence": item.oldest_available_sequence}}`.
|
|
2. `_stream_events` (`commands.py:1106`): change `{"events": [_message_dict(event) for event in events]}` to use `_event_row`.
|
|
3. New test in `clients/python/tests/test_cli.py`: `test_stream_events_renders_replay_gap` — monkeypatch `GatewayClient.connect` with the existing fake-connect pattern (see `test_cli.py:21-54`); the fake session's `stream_events` yields a `ReplayGap(requested_after_sequence=7, oldest_available_sequence=42)` followed by one normal `pb.MxEvent`; invoke `stream-events` through `CliRunner` and assert exit code 0, the output JSON contains one `replayGap` row with both sequences, and the normal event row follows it. This is the regression the review found missing.
|
|
4. Docs: extend the ReplayGap section of `docs/CrossLanguageSmokeMatrix.md:33-39` with a per-CLI rendering row (Python: `replayGap` JSON row — same shape as Rust) in the same commit.
|
|
|
|
**Verification.** From `clients/python`: `python -m pytest` (targeted first: `python -m pytest tests/test_cli.py -k replay_gap`). No proto change → no regen, no pinned `grpcio-tools` concern.
|
|
|
|
---
|
|
|
|
## CLI-36 — Go CLI `stream-events` silently destroys the ReplayGap signal `Medium` · `P0`
|
|
|
|
**Finding.** The Go library deliberately clears `EventResult.Event` on a gap and populates `EventResult.ReplayGap` (`clients/go/mxgateway/session.go:1036-1042`), but the CLI loop (`clients/go/cmd/mxgw-go/main.go:969-983`) only checks `result.Err` and then formats `result.Event`: JSON mode marshals a nil `*MxEvent` (empty object), text mode prints `0 MX_EVENT_FAMILY_UNSPECIFIED`. The gap's cursors — `requested_after_sequence` and `oldest_available_sequence`, the exact data an operator needs to resume — are discarded.
|
|
|
|
**Impact.** The typed signal the library was specifically built to preserve is destroyed one layer up; an operator resuming after an outage sees a meaningless zero row instead of the resume cursor. P0 per the roadmap (item 1, alongside GWC-25 and CLI-35).
|
|
|
|
**Design.** Add an `if result.IsReplayGap()` branch to the CLI loop that renders a dedicated gap row, conforming to the Rust CLI's canonical rendering (`mxgw-cli/src/main.rs:1019-1035`): text mode prints `REPLAY_GAP requested_after=<n> oldest_available=<n>`; JSON mode prints `{"replayGap": {"requestedAfterSequence": <n>, "oldestAvailableSequence": <n>}}` (marshal `result.ReplayGap` with `protojson` under a `replayGap` key, which yields exactly those camelCase fields). The gap row counts toward `-limit` like any other emitted row, mirroring Rust's `events` array accounting. Rejected alternative: printing the raw sentinel event as .NET/Java CLIs do — the Go library intentionally clears `Event`, so there is no sentinel left to print; reconstructing one would synthesize an event.
|
|
|
|
**Implementation.**
|
|
1. `clients/go/cmd/mxgw-go/main.go` `runStreamEvents` (loop at `:969-983`): after the `result.Err` check, insert:
|
|
- `if result.IsReplayGap()` → JSON: wrap `protojson.Marshal(result.ReplayGap)` as `{"replayGap":<bytes>}` (or a tiny struct with a `json.RawMessage`); text: `fmt.Fprintf(stdout, "REPLAY_GAP requested_after=%d oldest_available=%d\n", result.ReplayGap.GetRequestedAfterSequence(), result.ReplayGap.GetOldestAvailableSequence())`; then the shared `count++` / limit check, `continue`.
|
|
2. New test in `clients/go/cmd/mxgw-go/main_test.go`: `TestRunStreamEventsPrintsReplayGap` — reuse the fake-gateway pattern (`TestRunPingPlainText`, `main_test.go:195`): a fake `StreamEvents` server implementation sends one sentinel `MxEvent` with `replay_gap{requested_after_sequence: 7, oldest_available_sequence: 42}` set, then one normal data event, then returns. Run `stream-events` in text mode and assert stdout contains the typed line `REPLAY_GAP requested_after=7 oldest_available=42` and does **not** contain `0 MX_EVENT_FAMILY_UNSPECIFIED`; run again with `-json` and assert a `"replayGap"` object with both sequences.
|
|
3. Docs: the same `docs/CrossLanguageSmokeMatrix.md:33-39` per-CLI rendering row as CLI-35 (Go: typed `REPLAY_GAP` line / `replayGap` JSON row) — land the doc row once, covering both findings.
|
|
|
|
**Verification.** From `clients/go`: `gofmt -l .` (clean), `go build ./...`, `go test ./...` (targeted first: `go test ./cmd/mxgw-go -run TestRunStreamEventsPrintsReplayGap`).
|
|
|
|
---
|
|
|
|
## CLI-37 — Status-array validation must branch on `category`, per the proto's own rule `Medium` · `P1`
|
|
|
|
**Finding.** The wire contract (`src/ZB.MOM.WW.MxGateway.Contracts/Protos/mxaccess_gateway.proto:982-992`) states that `MxStatusProxy.success` "is NOT a boolean … carried verbatim for diagnostics; the authoritative success/failure of the operation is `category` (MX_STATUS_CATEGORY_OK marks success) … Clients should branch on `category`, not on a specific `success` value." Four clients branch only on `success`: Rust `clients/rust/src/error.rs:326` (`status.success == 0` — written fresh for CLI-03), Go `clients/go/mxgateway/status.go:4-6` (`GetSuccess() != 0`), Java `clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxStatuses.java:26`, Python `clients/python/src/zb_mom_ww_mxgateway/errors.py:141`. .NET requires **both** (`clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs:11-17`: `Success != 0 && Category is MxStatusCategory.Ok`). A reply with `success = 1, category = COMMUNICATION_ERROR` passes four clients and throws in .NET; `success = 0, category = OK` does the reverse.
|
|
|
|
**Impact.** The wire contract and four of five implementations disagree; identical gateway replies produce opposite outcomes per language. This is the same validation function CLI-38 touches — the two must land together as one conformance pass.
|
|
|
|
**Design — canonical behavior (this is a conformance exercise, not five fixes).** Per the proto comment — which pre-dates every implementation and records the worker's mapping intent (`success` is the raw 16-bit COM value carried verbatim; `category` is the worker's normalized verdict) — the canonical per-entry rule is:
|
|
|
|
> **An `MxStatusProxy` entry represents failure iff `category != MX_STATUS_CATEGORY_OK`. The `success` field is diagnostics only and never participates in the verdict.**
|
|
|
|
Edge decisions, uniform across all five: an absent entry (Go `nil`, Java `null`) remains success (nothing to report — preserves existing nil-tolerance); a **present** entry with `MX_STATUS_CATEGORY_UNSPECIFIED` (0) is a **failure** (an unmapped category is not proven OK, and the worker always maps one — an unspecified category on the wire is itself anomalous). Rejected alternatives: (a) declare the proto comment wrong and standardize on `success` — rejected: the comment is the contract, `success` is explicitly documented as a raw non-boolean diagnostic, and rewriting the contract to match four accidental implementations inverts the authority relationship; (b) require both, as .NET does today — rejected: makes `success = 0, category = OK` fail, directly contradicting "the authoritative … is `category`".
|
|
|
|
**Implementation** (one cross-client conformance commit, co-landed with CLI-38 since the same `EnsureMxAccessSuccess`-family functions change):
|
|
1. **Shared fixtures first** — add to `clients/proto/fixtures/behavior/command-replies/`: `write.status-category-error-success-set.reply.json` (protocol OK, one status with `success: 1, category: MX_STATUS_CATEGORY_COMMUNICATION_ERROR` → expected **failure**) and `write.status-category-ok-success-zero.reply.json` (`success: 0, category: MX_STATUS_CATEGORY_OK` → expected **success**); register both in `clients/proto/fixtures/behavior/manifest.json` and describe them in `docs/ClientBehaviorFixtures.md`.
|
|
2. **.NET** `clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs:11-17`: `IsSuccess` → `return status.Category is MxStatusCategory.Ok;` (drop the `Success != 0` conjunct); update the XML doc (line 8) accordingly.
|
|
3. **Go** `clients/go/mxgateway/status.go:4-6`: `StatusSucceeded` → `return status == nil || status.GetCategory() == pb.MxStatusCategory_MX_STATUS_CATEGORY_OK` (import the generated enum); rewrite the function comment.
|
|
4. **Java** `MxStatuses.java:25-27`: `succeeded` → `return status == null || status.getCategory() == MxStatusCategory.MX_STATUS_CATEGORY_OK;`; fix the class-level Javadoc (`:10-12`) and the `success()` accessor Javadoc (`:46-47`), which currently document the "non-zero success" convention.
|
|
5. **Python** `clients/python/src/zb_mom_ww_mxgateway/errors.py:140-141`: `if mx_status.success == 0:` → `if mx_status.category != pb.MX_STATUS_CATEGORY_OK:`.
|
|
6. **Rust** `clients/rust/src/error.rs:326`: `status.success == 0` → `status.category != MxStatusCategory::Ok as i32` (prost stores enums as `i32`); update the doc comment at `:313-314` ("A MXSTATUS_PROXY entry is treated as a failure when its `success` member is `0`") to the category rule; adjust the existing tests at `error.rs:410-452` that construct failing statuses via `success`.
|
|
7. Per-client tests: wire the two new fixtures into each fixture-driven suite (Go fixture event/reply tests, `clients/python/tests/`, `MxGatewayClientSessionTests.cs`, `MxGatewayClientSessionTests.java`, `clients/rust/tests/client_behavior.rs`), asserting failure/success exactly per the canonical rule.
|
|
8. Docs same-commit: `docs/ClientLibrariesDesign.md` Typed Command Parity section — extend the reply-validation sentence to state the per-item rule ("per-item `MxStatusProxy` failure iff `category != OK`; `success` is diagnostic only"); `docs/ClientBehaviorFixtures.md` (step 1).
|
|
|
|
**Verification.** All five, per CLAUDE.md: `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` + `dotnet test` (client tests); `gofmt`/`go build ./...`/`go test ./...` from `clients/go`; `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` from `clients/java` (**revert the regenerated `MxaccessGateway.java` churn afterward — no `.proto` changed**); `python -m pytest` from `clients/python`; `cargo fmt` + `cargo check --workspace` + `cargo test --workspace` + `cargo clippy --all-targets -- -D warnings` from `clients/rust`.
|
|
|
|
---
|
|
|
|
## CLI-38 — Align .NET/Go/Java on `hresult < 0` (lands prior CLI-08; cures the doc drift) `Medium` · `P1`
|
|
|
|
**Finding.** `docs/ClientLibrariesDesign.md:118-119` (the Typed Command Parity section added with CLI-04) claims every client "runs the same MXAccess-level reply validation (HRESULT `< 0` + per-item `MxStatusProxy`)". Actual: .NET `clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxCommandReplyExtensions.cs:34` (`reply.HasHresult && reply.Hresult != 0`), Go `clients/go/mxgateway/errors.go:187` (`reply.Hresult != nil && reply.GetHresult() != 0`), Java `clients/java/zb-mom-ww-mxgateway-client/.../MxGatewayErrors.java:50` (`reply.hasHresult() && reply.getHresult() != 0`) all use `!= 0`. Only Python (`errors.py:133`) and Rust (`error.rs:325`) implement the documented `< 0`.
|
|
|
|
**Impact.** A parity-preserving reply carrying a positive COM success code (`S_FALSE = 1`) errors in three languages and succeeds in two — and every CLI-04 helper (WriteSecured, AuthenticateUser, …) inherits the divergence. The load-bearing design doc describes behavior three clients don't have.
|
|
|
|
**Design.** **This finding is the vehicle that finally closes prior-cycle CLI-08** — the fix was fully designed in `archreview/remediation/50-clients.md` (CLI-08 section) two cycles ago and never landed; landing it now is strictly better than the alternative (correcting the doc to describe the divergence), because it makes the already-written doc true, closes CLI-08 and CLI-38 simultaneously, and completes the COM-correct semantics Rust/Python already have. Canonical rule, per COM semantics and the existing doc: **HRESULT failure iff `hresult` is present and `< 0`** (negative = failure; positive success codes like `S_FALSE` pass). Rejected alternative: doc-only correction — leaves a real cross-client behavioral divergence in the shipped typed surface and keeps CLI-08 open for a third cycle. Co-land with CLI-37: same functions, one conformance commit.
|
|
|
|
**Implementation.**
|
|
1. **.NET** `MxCommandReplyExtensions.cs:34`: `bool hResultFailure = reply.HasHresult && reply.Hresult < 0;`
|
|
2. **Go** `errors.go:187`: `if reply.Hresult != nil && reply.GetHresult() < 0 {`
|
|
3. **Java** `MxGatewayErrors.java:50`: `if (reply.hasHresult() && reply.getHresult() < 0) {`
|
|
4. **Shared fixtures**: add `clients/proto/fixtures/behavior/command-replies/write.hresult-s-false.reply.json` (protocol OK, `hresult: 1` → expected **success**) and `write.hresult-e-fail.reply.json` (`hresult: -2147467259` /0x80004005/ → expected **failure**) to the behavior manifest; wire into all five suites (Python/Rust already assert `< 0` in unit tests — the fixture locks it cross-client).
|
|
5. Docs/tracking same-commit: no change needed to `docs/ClientLibrariesDesign.md:118-119` — the change makes it true. Update the remediation tracking (`archreview/remediation/00-tracking.md` CLI-08 row → Done, noting it landed via CLI-38) and note the corrected semantics in the .NET/Go/Java README error sections (per the prior CLI-08 design).
|
|
|
|
**Verification.** Per language: `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` + client tests (new case: `hresult = 1` passes, negative fails); `go test ./...` from `clients/go`; `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` from `clients/java` (revert generated churn — no `.proto` changed). Python/Rust suites run unchanged as regression (`python -m pytest`; `cargo test --workspace`).
|
|
|
|
---
|
|
|
|
## CLI-39 — Bump versions off the already-published 0.1.2; guard the publish pipeline `Medium` · `P1`
|
|
|
|
**Finding.** All four non-Java clients pin the *already-published* 0.1.2: `clients/rust/Cargo.toml:3` (`[package]`) and `:28` (`[workspace.package]`, inherited by `crates/mxgw-cli` via `version.workspace = true`), `clients/python/pyproject.toml:9` + `clients/python/src/zb_mom_ww_mxgateway/version.py:3`, `clients/go/mxgateway/version.go:7`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj:22`. Java is at 0.2.0 (`clients/java/build.gradle:16`). Since the 0.1.2 publish, the public API changed incompatibly: Rust's `EventStream` item type became `Result<EventItem, Error>` and `Error` gained a variant; Python's `stream_events` yields `MxEvent | ReplayGap`; Go's `EventResult` gained `ReplayGap` and the `Events()` overflow contract changed to a terminal `ErrSlowConsumer`. The next `pack-clients.ps1` run either collides with the existing registry artifact or ships a different API under an identical version string.
|
|
|
|
**Impact.** Registry rejection at best; at worst an identical version labeling two different APIs for anyone who cached 0.1.2. Release hygiene for the whole client family.
|
|
|
|
**Design.** Bump **all five clients to 0.2.0** before the next publish. Rationale: the stream-surface changes are breaking, so under 0.x semver conventions (cargo treats 0.x minor as breaking) the right bump is minor, not patch — and 0.2.0 aligns every client on one number (Java is already there), simplifying the cross-language matrix and the e2e runner. Landing order: this is the **last** entry in the P0/P1 train — bump after CLI-35/36/37/38/45 so the published 0.2.0 carries the conformant behavior, not a third behavior set. Additionally, close the two publish-pipeline gaps this cycle re-confirmed: the tag-time Go version guard designed for CLI-21 but never implemented, and a registry-collision check in `scripts/pack-clients.ps1` (coordinate with the existing publish process: Gitea registry, credentials in `~/.zshenv`, Go module tags via `scripts/tag-go-module.ps1` with `clients/go/vX.Y.Z` prefixed tags). Rejected alternatives: (a) 0.1.3 patch bump — mislabels breaking changes and cargo consumers with `^0.1` would auto-upgrade into them; (b) per-language different bumps (0.2.0 Rust/Python, 0.1.3 Go/.NET) — legal but forfeits the alignment that makes the matrix and publish script auditable.
|
|
|
|
**Implementation.**
|
|
1. `clients/rust/Cargo.toml:3` and `:28`: `version = "0.2.0"` (both `[package]` and `[workspace.package]`; `crates/mxgw-cli` inherits; `CLIENT_VERSION` derives via `env!("CARGO_PKG_VERSION")` — no other Rust edit).
|
|
2. `clients/python/pyproject.toml:9` and `clients/python/src/zb_mom_ww_mxgateway/version.py:3`: `0.2.0` in both. Add/extend a test asserting `version.__version__` equals the version parsed from `pyproject.toml` (closes the CLI-26 residual drift mode the review noted — `test_cli.py:213` currently only asserts self-consistency).
|
|
3. `clients/go/mxgateway/version.go:7`: `ClientVersion = "0.2.0"`. The module tag `clients/go/v0.2.0` is created at publish time via `scripts/tag-go-module.ps1`.
|
|
4. `clients/dotnet/ZB.MOM.WW.MxGateway.Client/ZB.MOM.WW.MxGateway.Client.csproj:22`: `<Version>0.2.0</Version>`.
|
|
5. `clients/java/build.gradle:16`: stays `0.2.0` **only after verifying** (Gitea packages API) that Java 0.2.0 was not already published with the pre-remediation API; if it was, bump Java to 0.2.1 and record the exception in the packaging doc.
|
|
6. `scripts/tag-go-module.ps1`: implement the CLI-21 guard that was designed but skipped — after semver validation, read `clients/go/mxgateway/version.go` and fail the tag when `ClientVersion` ≠ the tag version (strip the `v`).
|
|
7. `scripts/pack-clients.ps1`: before each per-language publish step, query the Gitea package API for the target name+version and abort with a clear error when the artifact already exists (never force-overwrite). Reuse the verify-API calls the publish workflow already uses post-push.
|
|
8. Docs same-commit: grep `0\.1\.2` across `docs/ClientPackaging.md` and the five client READMEs; update any stale version references; note the version-bump-before-publish rule in `docs/ClientPackaging.md`'s versioning section.
|
|
|
|
**Verification.** Per-language builds prove the constants compile: `cargo check --workspace` + the existing `version.rs` test; `python -m pytest` (new pyproject-vs-`__version__` test); `go build ./...` + `go test ./...`; `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx`; `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` only if `build.gradle` changes (revert generated churn). Script checks: `pwsh -NoProfile -Command` dry-run of `tag-go-module.ps1` with a mismatched version (must fail) and a matched one (must pass); `pack-clients.ps1` guard exercised against the existing 0.1.2 artifacts (must refuse) — actual publish happens only when the release train is cut.
|
|
|
|
---
|
|
|
|
## CLI-40 — Port the exact-secret credential scrub to Rust/Java/.NET `Low` · `—`
|
|
|
|
**Finding.** The credential-redaction seam is uniform in name only. Go scrubs the *exact secret values* from any surfaced error (`clients/go/mxgateway/errors.go:16-66` `secretRedactingError`/`redactSecrets`, applied at `session.go:745`/`:836` for WriteSecured payloads and the AuthenticateUser password); Python does the same (`session.py:573-592` `_invoke_redacted`). Rust scrubs only whitespace-delimited tokens shaped like API keys (`clients/rust/src/error.rs:363-375` `redact_credentials`: `mxgw_` prefix or `bearer`); Java likewise (`clients/java/zb-mom-ww-mxgateway-client/.../MxGatewaySecrets.java:44-57`); .NET has **no library-level scrub** (exception text embeds `status.DiagnosticText` verbatim via `MxStatusProxyExtensions.ToDiagnosticSummary`; only the CLI redacts, `MxGatewayCliSecretRedactor.cs`). Yet `docs/ClientLibrariesDesign.md:123-127` and the Rust helper docs (`session.rs:876-883`) claim credentials can never reach exception text. The per-client tests pass because the fakes never echo the credential.
|
|
|
|
**Impact.** If the gateway or MXAccess echoes a credential back in `diagnostic_text` (which the client cannot prevent), Rust/Java/.NET surface it verbatim in exception messages — contradicting the documented guarantee. Low severity (requires a server-side echo), but the doc claims more than three clients deliver.
|
|
|
|
**Design — canonical behavior.** Every credential-bearing helper (`AuthenticateUser` password, `WriteSecured`/`WriteSecured2` string payloads) must scrub the **exact secret values** it was called with from any error text it surfaces, replacing each occurrence with `<redacted>` (the marker all five already use). This is defense-in-depth on top of the by-construction guarantee (exceptions carry reply-derived text, never the request). Go and Python define the reference semantics: wrap/post-process the error at the credential-bearing call site; typed-error matching (`errors.Is`/`except`/`catch`) must keep working through the redaction. Rejected alternative: downgrade the doc claim to "requests are never embedded in errors" — weaker guarantee than two clients already deliver and than the docs have promised since CLI-04; porting the seam is bounded work.
|
|
|
|
**Implementation.**
|
|
1. **Rust**: extend `MxAccessError` (and the error path used by credential helpers) to carry `secrets: Vec<String>`; in its `Display` (which already funnels through `redact_credentials`, `error.rs:363`), first replace each exact secret occurrence with `<redacted>`, then apply the existing pattern scrub. Credential helpers in `clients/rust/src/session.rs` (`authenticate_user`, `write_secured`, `write_secured2`) attach the secrets when mapping errors. Keep the helper-doc claim at `session.rs:876-883` — it becomes true.
|
|
2. **Java**: add `MxGatewaySecrets.redactExact(String message, String... secrets)` (exact-substring replacement, null/blank-tolerant) alongside the pattern scrub; route the credential commands in `MxGatewaySession.java` (`authenticateUser`, `writeSecured`, `writeSecured2`) through a private `invokeCommandRedacted(command, String... secrets)` that catches `MxGatewayException`, rebuilds the message via `redactExact`, and rethrows the **same exception type** (mirror Python's `_invoke_redacted`).
|
|
3. **.NET**: introduce a library-level equivalent in `MxGatewaySession.cs`: the credential helpers (`AuthenticateUserAsync`, `WriteSecuredAsync`, `WriteSecured2Async`) wrap their invoke+validate in a catch that rethrows the same exception type with each exact secret replaced by `<redacted>` in `Message` (reuse the `<redacted>` marker; factor the replacement into a small internal `MxGatewaySecretRedaction` helper so the CLI's `MxGatewayCliSecretRedactor` can share it).
|
|
4. **Shared fixture**: add `clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential.reply.json` — an MXAccess-failure reply whose `diagnostic_text` embeds the fixture credential string; every client's suite asserts the surfaced error message does **not** contain the credential and does contain `<redacted>`. Register in the manifest + `docs/ClientBehaviorFixtures.md`.
|
|
5. Docs: none beyond the fixture doc — the design-doc claim becomes accurate.
|
|
|
|
**Verification.** `cargo fmt`/`check`/`test`/`clippy -D warnings` from `clients/rust`; `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` from `clients/java` (revert generated churn); `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` + client tests; Go/Python suites re-run the new shared fixture as regression.
|
|
|
|
---
|
|
|
|
## CLI-41 — Uniform malformed-reply contract for the id/handle-returning helpers `Low` · `—`
|
|
|
|
**Finding.** A gateway reply that omits the typed payload yields five different behaviors. `AuthenticateUser`: Go falls back to `GetReturnValue().GetInt32Value()` → silent `0` when both are absent (`clients/go/mxgateway/session.go:807-816`); Python reads `reply.authenticate_user.user_id` → proto3 default `0`, no fallback, no error (`clients/python/src/zb_mom_ww_mxgateway/session.py:713`); Java falls back like Go → `0` (`MxGatewaySession.java:868-880`); .NET `reply.AuthenticateUser?.UserId ?? reply.ReturnValue.Int32Value` → `NullReferenceException` when both are absent (`clients/dotnet/.../MxGatewaySession.cs:1289`); Rust returns a typed `Error::MalformedReply` (`clients/rust/src/session.rs:1073-1089`) — but Rust's own `add_buffered_item_handle` (`:1057-1071`) *does* fall back to `return_value`, so Rust is inconsistent internally.
|
|
|
|
**Impact.** Silent `0` from `AuthenticateUser` is the worst mode — callers feed it into `WriteSecured` as a real user id. The NRE is a crash on a malformed-but-received reply.
|
|
|
|
**Design — canonical behavior.** For every helper that extracts a scalar from a reply (`AuthenticateUser`, `ArchestrAUserToId`, `AddBufferedItem`, and the existing `AddItem2`-style handle extractors):
|
|
|
|
> **Prefer the typed payload; when it is absent, fall back to `return_value` only if `return_value` is actually present with the expected variant; when neither is present, raise/return a typed malformed-reply error. Never surface a proto3 default `0`, never NRE.**
|
|
|
|
This is Rust's `add_buffered_item_handle` pattern (payload → present `return_value` → `MalformedReply`), generalized. The `return_value` fallback is kept because it is the documented compatibility path for replies from workers that populate only the legacy field; the typed error replaces only the "both absent" case. Rejected alternatives: (a) Rust's stricter payload-or-error (no fallback, current `authenticate_user_id`) — breaks the legacy-`return_value` compatibility the other four clients and Rust's own handle extractors honor; (b) documenting the divergence — a silent-`0` user id is a correctness hazard, not a doc gap.
|
|
|
|
**Implementation.**
|
|
1. **Go** `clients/go/mxgateway/session.go`: add a `MalformedReplyError{Op string, Detail string}` type to `errors.go` (Error/Unwrap in the house style). In `AuthenticateUser` (`:807`), `ArchestrAUserToId`, and `AddBufferedItem`: check the typed payload; else `if reply.ReturnValue != nil` use it; else return the typed error.
|
|
2. **Python** `clients/python/src/zb_mom_ww_mxgateway/session.py` + `errors.py`: add `MalformedReplyError(MxGatewayError)`. In `authenticate_user` (`:713`), `archestra_user_to_id`, `add_buffered_item`: `if reply.HasField("authenticate_user")` → typed payload; `elif reply.HasField("return_value")` and the value oneof is `int32_value` → fallback; else raise.
|
|
3. **Java** `MxGatewaySession.java:868-880` (+ `archestrAUserToId`, `addBufferedItem`): `if (reply.hasAuthenticateUser())` → payload; `else if (reply.hasReturnValue())` → fallback; else throw a new `MxGatewayMalformedReplyException extends MxGatewayException`.
|
|
4. **.NET** `MxGatewaySession.cs:1289` (+ the archestra/buffered equivalents at `:1332`/`:935`): `reply.AuthenticateUser?.UserId ?? reply.ReturnValue?.Int32Value ?? throw new MxGatewayMalformedReplyException(...)` (new exception type deriving from `MxGatewayException`).
|
|
5. **Rust** `clients/rust/src/session.rs:1073-1089`: add the `return_value` fallback to `authenticate_user_id` and `archestra_user_id`, matching `add_buffered_item_handle` — payload → `return_value` via `int32_reply_value` → `Error::MalformedReply`.
|
|
6. **Shared fixtures**: `clients/proto/fixtures/behavior/command-replies/authenticate-user.missing-payload.reply.json` (protocol OK, no payload, no `return_value` → expected typed error) and `authenticate-user.return-value-only.reply.json` (`return_value.int32_value = 7`, no typed payload → expected `7`); manifest + `docs/ClientBehaviorFixtures.md` + all five suites.
|
|
7. Docs: one sentence in `docs/ClientLibrariesDesign.md`'s Typed Command Parity section stating the payload → return_value → typed-error contract.
|
|
|
|
**Verification.** All five per CLAUDE.md: dotnet slnx build + tests; `go test ./...`; `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` (revert generated churn); `python -m pytest`; `cargo fmt`/`check`/`test`/`clippy -D warnings`.
|
|
|
|
---
|
|
|
|
## CLI-42 — Document the vendored Rust proto layout (CLI-02's missing doc half) `Low` · `P1 (doc batch)`
|
|
|
|
**Finding.** CLI-02's code fix landed (repo-path-first/vendored-fallback `build.rs`, `clients/rust/protos/*.proto` vendored and `include`d in `Cargo.toml:20`, `--no-verify` removed from packaging) but the documentation half was skipped: `clients/rust/README.md:21-25` still says `build.rs` reads only `../../src/ZB.MOM.WW.MxGateway.Contracts/Protos`, and `docs/ClientPackaging.md`'s Rust section (`:114-127`) never mentions `clients/rust/protos/` (repo-wide grep for "vendored" in `docs/*.md` → zero hits). The vendored copies are load-bearing published build inputs with a refresh obligation on every `.proto` change, currently enforced only by `scripts/check-codegen.ps1` Check 3 and a `build.rs` comment. CLAUDE.md's docs-in-same-commit rule was not met.
|
|
|
|
**Impact.** Anyone editing the contracts protos has no prose telling them the Rust crate ships its own copies that must be refreshed; the packaging doc describes a crate layout that no longer matches what `cargo package` ships.
|
|
|
|
**Design.** Pure documentation: describe the resolution order and the refresh rule in both places the review names. No code. Batched with the other P1 doc work.
|
|
|
|
**Implementation.**
|
|
1. `clients/rust/README.md:21-25`: rewrite the generation paragraph — `build.rs` resolves protos repo-path-first (`../../src/ZB.MOM.WW.MxGateway.Contracts/Protos`, keeping in-repo `.proto` edits live) and falls back to the vendored copies in `clients/rust/protos/` (shipped in the published crate via `Cargo.toml`'s `include`, making the tarball self-sufficient — this is what `cargo package` verification exercises). State the refresh rule: any change to the Contracts protos must copy the changed files into `clients/rust/protos/` in the same commit; `scripts/check-codegen.ps1` (Check 3) fails on byte drift.
|
|
2. `docs/ClientPackaging.md` Rust section (after the `build.rs` sentence around `:116-118`): add the same vendored-layout paragraph plus a note that `cargo package`/`cargo publish` run **with** verification (no `--no-verify`) precisely because the vendored protos make the standalone build possible.
|
|
|
|
**Verification.** `grep -rn "protos/" clients/rust/README.md docs/ClientPackaging.md` shows the vendored dir documented in both; `grep -i vendored docs/ClientPackaging.md clients/rust/README.md` non-empty. No build required (docs only).
|
|
|
|
---
|
|
|
|
## CLI-43 — Java style guide still prescribes "Java 21 preferred" `Low` · `—`
|
|
|
|
**Finding.** `docs/style-guides/JavaStyleGuide.md:8` — "Target the Java version defined by the client build, with Java 21 preferred." CLAUDE.md declares `docs/style-guides/` authoritative, and this line contradicts the shipped `toolchain 17` / `options.release = 17` build (`clients/java/build.gradle`) and the CLI-12-corrected docs. (Historical plan docs also say 21 but are archival — leave them.)
|
|
|
|
**Impact.** An authoritative guide instructing contributors toward the wrong language level for the Ignition 8.3 target.
|
|
|
|
**Design.** One-line doc fix, mirroring CLI-12's wording. No code.
|
|
|
|
**Implementation.**
|
|
1. `docs/style-guides/JavaStyleGuide.md:8`: replace with "Target Java 17 (the Ignition 8.3 baseline; the client build enforces `options.release = 17` with a Gradle toolchain 17). Code must compile and run on 17; newer JDKs may host the build."
|
|
|
|
**Verification.** `grep -rn "Java 21" docs/style-guides/` → empty. No build (docs only).
|
|
|
|
---
|
|
|
|
## CLI-44 — Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` `Low` · `—`
|
|
|
|
**Finding.** When `stream.Recv()` returns a real terminal error while the 16 data slots are full, the Recv-error path (`clients/go/mxgateway/session.go:1051-1057`) reuses `sendEventResult`, whose overflow branch (`:1082-1101`) fires first: it substitutes `ErrSlowConsumer` for the caller's `GatewayError{Err: err}`. The consumer gets a loud terminal error (good) with the wrong root cause — the real gRPC status is lost.
|
|
|
|
**Impact.** Misleading diagnostics on a narrow race (full buffer + genuine stream failure), not silent loss — hence Low.
|
|
|
|
**Design.** Terminal sends must bypass the overflow substitution and use the reserved slot directly with the caller's error. The reserved slot (`eventBufferReservedSlots`, `session.go:47-56`) exists exactly to guarantee one terminal result is always deliverable; the sole-producer invariant (CLI-01's design) means at most one terminal send ever races for it. Rejected alternative: enlarging the reserve to two slots so both errors can be delivered — the consumer only ever acts on the first terminal error; delivering two contradicting terminal results complicates the contract for no diagnostic gain.
|
|
|
|
**Implementation.**
|
|
1. `clients/go/mxgateway/session.go`: add a small `sendTerminalEventResult(results chan<- EventResult, result EventResult)` doing a non-blocking `select { case results <- result: default: }` (comment: the reserved slot guarantees the send unless a terminal result was already enqueued). Use it in the Recv-error path (`:1051-1057`) instead of `sendEventResult` (the stream has already ended — no `cancel()` needed beyond the deferred cleanup; keep the existing `cancel` call if one is required for stream teardown symmetry). The overflow branch in `sendEventResult` is unchanged.
|
|
2. New test in `clients/go/mxgateway/client_session_test.go`: `TestEventsFullBufferTerminalErrorKeepsRootCause` — fill all 16 data slots without draining (reuse the CLI-01 slow-consumer test scaffolding at `:150-185`), then make the fake stream's `Recv` return a non-EOF gRPC error; drain and assert the final `EventResult.Err` unwraps to the gRPC status error and `errors.Is(err, ErrSlowConsumer)` is **false**.
|
|
3. Docs: adjust the `Events`/`EventsAfter` doc comments if they state that a full buffer always yields `ErrSlowConsumer` (the contract becomes: overflow yields `ErrSlowConsumer`; a genuine stream error is reported as itself even under overflow).
|
|
|
|
**Verification.** From `clients/go`: `gofmt -l .`, `go build ./...`, `go test ./...` (targeted: `go test ./mxgateway -run TestEventsFullBufferTerminalErrorKeepsRootCause`).
|
|
|
|
---
|
|
|
|
## CLI-45 — Standardize CLI credential env-var name; fail fast on missing/empty passwords `Low` · `P1`
|
|
|
|
**Finding.** The new `authenticate-user` CLI credential flags diverge per language. Go (`clients/go/cmd/mxgw-go/main.go:441-457`): `-password`/`-password-env`, default env `MXGATEWAY_VERIFY_PASSWORD`, but an unresolved (empty) password is **silently sent to the wire**. Java (`clients/java/zb-mom-ww-mxgateway-cli/.../MxGatewayCli.java:1136-1160`): same flag/env names, and an unset env falls back to `resolvedPassword = ""` — also sent as an empty credential. Rust (`clients/rust/crates/mxgw-cli/src/main.rs:136-155`): same names, missing → `Error::InvalidArgument` (conformant). Python (`commands.py:832-847` `_resolve_password`): no default env name — `--password-env` must be passed explicitly; missing → `click.UsageError`. .NET (`MxGatewayClientCli.cs:349-388`): different flag names (`--verify-user-password`/`--verify-user-password-env`) and a different default env (`MXGATEWAY_VERIFY_USER_PASSWORD`); missing → `ArgumentException`. Subcommand coverage also diverges (.NET exposes all nine new commands; Rust adds unregister + the two credential commands; Go/Python/Java add only `write-secured` + `authenticate-user`).
|
|
|
|
**Impact.** The same operator workflow (`export MXGATEWAY_VERIFY_PASSWORD=… && <cli> authenticate-user …`) works in three CLIs, needs an explicit flag in Python, and needs a different variable in .NET — and Go/Java silently authenticate with an empty password, turning a misconfigured env into a real (failing or, worse, succeeding) MXAccess authentication attempt.
|
|
|
|
**Design — canonical behavior.** One CLI credential contract for all five:
|
|
|
|
> Flags `--password` (explicit value; discouraged — stays out of shell history via the env path) and `--password-env` (name of the environment variable; **default `MXGATEWAY_VERIFY_PASSWORD`**). Resolution order: flag, then env. When the resolved credential is **missing or empty**, the CLI fails fast with a usage error naming the flag and the env var — it never sends an empty password to the wire and never echoes the value.
|
|
|
|
`MXGATEWAY_VERIFY_PASSWORD` wins as the canonical name because three of five CLIs and their docs already use it — changing .NET (one CLI, with aliases) is the smallest blast radius; rejected alternative: standardizing on .NET's `MXGATEWAY_VERIFY_USER_PASSWORD` would churn three CLIs plus the smoke matrix for no benefit. The fail-fast rule is CLI argument validation, not a parity violation: the *library* still transmits whatever credential it is given (MXAccess parity preserved); only the operator tool refuses to fabricate an empty one — matching what Rust/Python/.NET already do. Subcommand-coverage leveling is out of scope here (larger feature work); the deltas get documented instead.
|
|
|
|
**Implementation.**
|
|
1. **Go** `clients/go/cmd/mxgw-go/main.go` (`runAuthenticateUser`, after the resolution block at `:454-457`): `if resolvedPassword == "" { return fmt.Errorf("a password is required via -password or the %s environment variable", *passwordEnv) }` — before dialing.
|
|
2. **Java** `MxGatewayCli.java` `AuthenticateUserCommand.call()` (`:1151-1159`): replace the `resolvedPassword = ""` fallback with a picocli `ParameterException` (via `common.spec.commandLine()`) when the resolved value is null or blank, message naming `--password` and the `--password-env` variable.
|
|
3. **Python** `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py`: give the `--password-env` click option the default `"MXGATEWAY_VERIFY_PASSWORD"` (both `authenticate-user` and any secured-write command that shares the option); `_resolve_password` keeps its `click.UsageError` (already conformant once the default exists).
|
|
4. **.NET** `MxGatewayClientCli.cs` (`TryResolveVerifyUserPassword`/`ResolveVerifyUserPassword`, `:349-388`): accept `--password` and `--password-env` as the primary names with default env `MXGATEWAY_VERIFY_PASSWORD`; keep `--verify-user-password`, `--verify-user-password-env`, and the `MXGATEWAY_VERIFY_USER_PASSWORD` env as deprecated aliases/fallbacks for one release (resolution order: `--password`, `--verify-user-password`, env named by `--password-env` (default `MXGATEWAY_VERIFY_PASSWORD`), legacy `MXGATEWAY_VERIFY_USER_PASSWORD`). The existing `ArgumentException` fail-fast stays; ensure the empty-string case (flag or env set but empty) also fails.
|
|
5. **Rust** `mxgw-cli/src/main.rs:143-152`: verify the empty-string env case — if `std::env::var` returns an empty string it must be treated as missing (add the check if absent). Otherwise already conformant.
|
|
6. Tests (new, per CLI): Go `TestRunAuthenticateUserRejectsEmptyPassword` in `main_test.go` (env unset → error mentioning `MXGATEWAY_VERIFY_PASSWORD`, no RPC attempted — no fake server needed); Java a picocli-level test invoking `authenticate-user` without credential and asserting the usage error; Python a `test_cli.py` case asserting the default env name is honored (monkeypatch `os.environ`) and that missing → `UsageError`; .NET CLI tests asserting the new names, the alias fallback, and the empty-value failure; Rust a case for empty-string env → `InvalidArgument`.
|
|
7. Docs same-commit: `docs/CrossLanguageSmokeMatrix.md` — document the canonical flag/env contract in the smoke-command section and add a per-CLI subcommand-coverage table noting the current deltas (.NET: all nine; Rust: unregister + credential pair; Go/Python/Java: `write-secured` + `authenticate-user`) so the divergence is at least specified until a leveling pass; client READMEs' `authenticate-user` sections updated where they name the env var (.NET README gains the rename + deprecation note).
|
|
|
|
**Verification.** Go: `gofmt -l .`, `go build ./...`, `go test ./...`. Java: `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` from `clients/java` (**revert the `MxaccessGateway.java` regen churn — no `.proto` changed**). Python: `python -m pytest` from `clients/python`. .NET: `dotnet build clients/dotnet/ZB.MOM.WW.MxGateway.Client.slnx` + client/CLI tests. Rust: `cargo fmt`, `cargo check --workspace`, `cargo test --workspace`, `cargo clippy --all-targets -- -D warnings` from `clients/rust`.
|