fix(clients): resolve 2026-06-18 array-write review findings
- Client.Dotnet-030: add advise-supervisory to IsKnownGatewayCommand (was dead/unreachable, exit 2) - Client.Go-035/036/037: usage+README list advise-supervisory; add session-id guard test; fix write2 README wording - Client.Python-037/038: drop regressed 'scaffold' from pyproject; add advise-supervisory CLI tests - Client.Rust-039/040: document write_array_elements/advise-supervisory in design doc; pin outer MxValue data_type==0 - Client.Java-049/050/051: sync CLIENT_VERSION to 0.1.2; add advise-supervisory test; guard negative uint32 inputs (pending windev gradle verification) Client READMEs also updated for Server-057 add-family normalization wording. .NET/Go/Python/Rust verified green locally; Java pending windev.
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-06-18 |
|
||||
| Commit reviewed | `88915c3` |
|
||||
| Status | Re-reviewed |
|
||||
| Open findings | 3 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -929,12 +929,14 @@ BrowseChildrenReply reply = galaxy.browseChildren(
|
||||
| Severity | Low |
|
||||
| Category | Code organization & conventions |
|
||||
| Location | `clients/java/build.gradle:16`, `clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientVersion.java:12`, `clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java:59,89`, `clients/java/README.md:399` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Commit `88915c3` (`chore(clients): bump all five clients 0.1.1 -> 0.1.2`) incremented `build.gradle` `version = '0.1.2'` but left `MxGatewayClientVersion.CLIENT_VERSION = "0.1.1"` unchanged. The two CLI test assertions that check the version string also still assert `0.1.1` (lines 59 and 89 of `MxGatewayCliTests.java`), and the `README.md` Maven dependency example at line 399 shows `:0.1.1`. The published Gradle artifact carries version `0.1.2` (from `build.gradle`) while the `version` CLI command reports `mxgateway-java 0.1.1` and the README tells a consumer to depend on `:0.1.1`. Same class of version drift as the resolved Client.Java-044 (where `0.1.0` vs `0.1.1` was the split) — the fix for Client.Java-044 bumped `CLIENT_VERSION` to `"0.1.1"` but the `build.gradle` bump to `0.1.2` was not accompanied by a matching `MxGatewayClientVersion` update.
|
||||
|
||||
**Recommendation:** Bump `CLIENT_VERSION` to `"0.1.2"` in `MxGatewayClientVersion.java`, update the two `MxGatewayCliTests` assertions from `0.1.1` to `0.1.2`, and update the `README.md` dependency example coordinate to `:0.1.2`. Consider sourcing `CLIENT_VERSION` from a Gradle-generated resource file (e.g. via `processResources` task writing `version.properties`) so the two version strings cannot drift again.
|
||||
|
||||
**Resolution:** 2026-06-18 — Confirmed: `build.gradle` already at `0.1.2` while `CLIENT_VERSION` was still `"0.1.1"` and test assertions/README matched the old value. Bumped `CLIENT_VERSION` to `"0.1.2"` in `MxGatewayClientVersion.java`, updated both version assertions in `MxGatewayCliTests.java` (plain-text and JSON paths), and updated the Maven dependency coordinate in `README.md` to `:0.1.2`. No new test needed — the two existing assertions (`versionCommandPrintsProtocolVersions`, `versionCommandPrintsJson`) now exercise the corrected value. (pending windev gradle verification)
|
||||
|
||||
### Client.Java-050
|
||||
|
||||
| Field | Value |
|
||||
@@ -942,12 +944,14 @@ BrowseChildrenReply reply = galaxy.browseChildren(
|
||||
| Severity | Low |
|
||||
| Category | Testing coverage |
|
||||
| Location | `clients/java/zb-mom-ww-mxgateway-cli/src/main/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCli.java:1046-1068` (new `AdviseSupervisoryCommand`), `clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java:1306-1313` (stub) |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Commit `9eedf9d` added the `advise-supervisory` CLI subcommand (`AdviseSupervisoryCommand`) to all language client CLIs. The Java `FakeSession.adviseSupervisoryRaw` stub was added to `MxGatewayCliTests` but no test exercises the new subcommand path. There is no test that calls `execute(factory, "advise-supervisory", "--session-id", "s", "--server-handle", "1", "--item-handle", "2")` and asserts the command routes through `session.adviseSupervisoryRaw`, produces a non-zero exit code on failure, or emits the correct JSON / text output. The `adviseCalled` field shared with `adviseRaw` means even an indirect smoke path that calls `advise` could mask a missing `adviseSupervisory` wire. Every other new CLI subcommand in this diff has a dedicated CLI-level test (the `writeArrayElements` helper has a session-level test in `MxGatewayClientSessionTests`).
|
||||
|
||||
**Recommendation:** Add a `@Test void adviseSupervisoryCommandCallsAdviseSupervisoryRaw()` to `MxGatewayCliTests` that exercises the subcommand via `execute(factory, "advise-supervisory", "--session-id", "s", "--server-handle", "12", "--item-handle", "34")` and asserts exit code 0, that `factory.client.session.adviseCalled` (or a dedicated `adviseSupervisoryCalled` boolean) is true, and that the output contains the reply kind string `MX_COMMAND_KIND_ADVISE_SUPERVISORY`. Consider renaming `adviseCalled` to `adviseSupervisoryCalled` for the `adviseSupervisoryRaw` stub (a separate `adviseCalled` for `adviseRaw`) to prevent future tests from masking each other.
|
||||
|
||||
**Resolution:** 2026-06-18 — Confirmed: `adviseSupervisoryRaw` stub existed in `FakeSession` but shared `adviseCalled` with the plain `adviseRaw` stub, and no test exercised the `advise-supervisory` subcommand path. Added a dedicated `adviseSupervisoryCalled` boolean field to `FakeSession` and wired it to the `adviseSupervisoryRaw` stub (severing the shared flag that masked routing). Added `adviseSupervisoryCommandCallsAdviseSupervisoryRaw` test in `MxGatewayCliTests.java` that invokes `execute(factory, "advise-supervisory", "--session-id", "session-cli", "--server-handle", "12", "--item-handle", "34", "--json")` and asserts exit code 0, `adviseSupervisoryCalled` is true, `adviseCalled` is false (verifying routing isolation), and output contains `"kind":"MX_COMMAND_KIND_ADVISE_SUPERVISORY"`. (pending windev gradle verification)
|
||||
|
||||
### Client.Java-051
|
||||
|
||||
| Field | Value |
|
||||
@@ -955,7 +959,7 @@ BrowseChildrenReply reply = galaxy.browseChildren(
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java:622-657` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `writeArrayElements` accepts `int totalLength` and `Map<Integer, MxValue> elements` whose keys are plain Java `int`. The proto fields `MxSparseArray.total_length` and `MxSparseElement.index` are both `uint32`. Java's protobuf runtime maps `uint32` to `int` (Java has no unsigned primitive), so passing a negative value to `setTotalLength(int)` or `setIndex(int)` silently sets the wire field to the two's-complement reinterpretation (e.g. `-1` → `4294967295`). The gateway will likely reject the resulting request with `INVALID_ARGUMENT`, but the error message will reference a large `uint32` value rather than the caller's negative `int`, making the failure hard to diagnose. The Javadoc states "supplied indices must be within `[0, totalLength)`" and "`totalLength` is required" but does not state what happens with negative inputs, and no `IllegalArgumentException` is thrown. All other language clients use unsigned types (`uint`, `uint32`, `u32`) that prevent negatives at the type level; Java cannot replicate that, so explicit validation is the correct substitute. The Python client is similarly unvalidated and its docstring explicitly defers to the gateway for rejection — but Python's `grpc` runtime raises an internal exception on negative `uint32` fields before the network call, so it fails more obviously than Java's silent wire wrap.
|
||||
|
||||
@@ -975,3 +979,5 @@ for (Map.Entry<Integer, MxValue> entry : elements.entrySet()) {
|
||||
```
|
||||
|
||||
Add a test in `MxGatewayClientSessionTests` asserting both `IllegalArgumentException` paths (negative `totalLength`, negative/out-of-range index). Duplicate-index detection can be left to the gateway (the proto `repeated` field allows duplicates, and the gateway can sort out semantics).
|
||||
|
||||
**Resolution:** 2026-06-18 — Confirmed: `writeArrayElements` passed negative `int` values straight to `setTotalLength`/`setIndex` with no guard, silently producing large `uint32` wire values. Added `if (totalLength <= 0) throw new IllegalArgumentException(...)` and a per-entry `if (idx < 0 || idx >= totalLength) throw new IllegalArgumentException(...)` loop before the proto builder in `MxGatewaySession.writeArrayElements`. Updated Javadoc to document the new `@throws IllegalArgumentException` contract and the uint32 unsigned-type rationale. Added two tests in `MxGatewayClientSessionTests`: `writeArrayElementsRejectsNonPositiveTotalLength` (covers negative and zero `totalLength`) and `writeArrayElementsRejectsOutOfRangeIndex` (covers negative index, index equal to `totalLength`, and index above `totalLength`). (pending windev gradle verification)
|
||||
|
||||
Reference in New Issue
Block a user