diff --git a/clients/dotnet/README.md b/clients/dotnet/README.md index d6eca9b..71ecb15 100644 --- a/clients/dotnet/README.md +++ b/clients/dotnet/README.md @@ -189,8 +189,9 @@ array before forwarding to the worker. Indices not listed in `elements` are written as the element type's default — this is a **reset**, not a preserve; current values at those positions are discarded. `totalLength` is required and must match the declared length of the array attribute. Bare-name array items -(`Area001.Pump001.Speed`) are auto-normalized to the `[]` form at `AddItem` -so the array attribute accepts the write. +(`Area001.Pump001.Speed`) are auto-normalized to the `[]` form across the whole +add family — `AddItem`, `AddItem2`, `AddItemBulk`, and `AddBufferedItem` — so the +array attribute accepts the write. ## CLI Usage diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs index 93e69e1..e8d56a8 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs @@ -2028,6 +2028,7 @@ public static class MxGatewayClientCli or "register" or "add-item" or "advise" + or "advise-supervisory" or "subscribe-bulk" or "unsubscribe-bulk" or "read-bulk" diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientCliTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientCliTests.cs index cf92ebe..bd39120 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientCliTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewayClientCliTests.cs @@ -82,6 +82,53 @@ public sealed class MxGatewayClientCliTests Assert.Equal(string.Empty, error.ToString()); } + /// + /// Client.Dotnet-030: advise-supervisory was present in the command + /// dispatch table but absent from 's + /// IsKnownGatewayCommand guard, so the guard intercepted it first and + /// returned exit code 2 "Unknown command" before dispatch could run. This + /// test asserts the command is recognized (exit ≠ 2, stderr contains no + /// "Unknown command") and reaches the dispatch handler (exit 0, reply written + /// to stdout). + /// + [Fact] + public async Task RunAsync_AdviseSupervisory_IsRecognizedAndReachesDispatch() + { + using var output = new StringWriter(); + using var error = new StringWriter(); + FakeCliClient fakeClient = new(); + fakeClient.InvokeReplies.Enqueue(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.AdviseSupervisory, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + + int exitCode = await MxGatewayClientCli.RunAsync( + [ + "advise-supervisory", + "--endpoint", + "http://localhost:5000", + "--api-key", + "test-api-key", + "--session-id", + "session-fixture", + "--server-handle", + "12", + "--item-handle", + "34", + "--json", + ], + output, + error, + _ => fakeClient); + + Assert.DoesNotContain("Unknown command", error.ToString()); + Assert.Equal(0, exitCode); + Assert.Contains("MX_COMMAND_KIND_ADVISE_SUPERVISORY", output.ToString()); + Assert.Equal(string.Empty, error.ToString()); + } + /// Verifies that error output redacts sensitive API key values. [Fact] public async Task RunAsync_ErrorOutput_RedactsApiKey() diff --git a/clients/go/README.md b/clients/go/README.md index 2a4ac74..1c66881 100644 --- a/clients/go/README.md +++ b/clients/go/README.md @@ -133,8 +133,8 @@ _, err := client.Invoke(ctx, &pb.MxCommandRequest{ err = session.Write(ctx, serverHandle, itemHandle, value, userID) ``` -The CLI exposes the same command as `advise-supervisory`, and `write` / -`write2` take `--user-id`. +The CLI exposes the same command as `advise-supervisory`, and `write` +takes `-user-id`. ### Array writes replace the whole array @@ -167,9 +167,10 @@ err = session.WriteArrayElements( ) ``` -`AddItem` (and `AddItem2`) now auto-normalize a bare attribute name to the `[]` -array address form expected by MXAccess, so callers do not need to append `[]` -themselves. Both forms are accepted; duplicates are deduplicated by the gateway. +`AddItem`, `AddItem2`, `AddItemBulk`, and `AddBufferedItem` auto-normalize a +bare array attribute name to the `[]` array address form expected by MXAccess, +so callers do not need to append `[]` themselves. Both forms are accepted; +duplicates are deduplicated by the gateway. ## Galaxy Repository browse @@ -334,6 +335,7 @@ Every subcommand wired into the CLI. All accept the common flags | `register` | Register a client name on a session (`-session-id`, `-client-name`). | | `add-item` | Add an item handle (`-session-id`, `-server-handle`, `-item`). | | `advise` | Advise (subscribe) one item (`-session-id`, `-server-handle`, `-item-handle`). | +| `advise-supervisory` | Advise one item supervisory — required before a user-id-attributed plain `write`. | | `subscribe-bulk` | Advise many items in one call. | | `unsubscribe-bulk` | Unadvise many item handles in one call. | | `read-bulk` | Read snapshots for many item handles in one call. | diff --git a/clients/go/cmd/mxgw-go/main.go b/clients/go/cmd/mxgw-go/main.go index d2dfda1..4d8e4c8 100644 --- a/clients/go/cmd/mxgw-go/main.go +++ b/clients/go/cmd/mxgw-go/main.go @@ -1295,7 +1295,7 @@ type protojsonMessage interface { } func writeUsage(writer io.Writer) { - fmt.Fprintln(writer, "usage: mxgw-go ") + fmt.Fprintln(writer, "usage: mxgw-go ") } // batchEOR is the end-of-result sentinel emitted to stdout after every command diff --git a/clients/go/cmd/mxgw-go/main_test.go b/clients/go/cmd/mxgw-go/main_test.go index 31b9194..f78879b 100644 --- a/clients/go/cmd/mxgw-go/main_test.go +++ b/clients/go/cmd/mxgw-go/main_test.go @@ -558,6 +558,16 @@ func TestRunStreamEventsRequiresSessionID(t *testing.T) { } } +// TestRunAdviseSupervisoryRequiresSessionID pins the session-id guard so +// advise-supervisory fails fast before dialing when no session id is supplied. +func TestRunAdviseSupervisoryRequiresSessionID(t *testing.T) { + var stdout, stderr bytes.Buffer + err := runWithIO(t.Context(), []string{"advise-supervisory", "-plaintext", "-api-key", "test"}, &stdout, &stderr) + if err == nil || !strings.Contains(err.Error(), "session-id is required") { + t.Fatalf("advise-supervisory without -session-id error = %v", err) + } +} + // TestRunWriteBulkVariantRejectsMismatchedHandlesAndValues pins the len-mismatch // guard so a write-bulk with unequal item-handles / values counts fails fast // before any dial. diff --git a/clients/java/README.md b/clients/java/README.md index 0b87b2f..c20b3da 100644 --- a/clients/java/README.md +++ b/clients/java/README.md @@ -144,8 +144,9 @@ array before forwarding to the worker. Indices not listed in the map are written as the element type's default — this is a **reset**, not a preserve; current values at those positions are discarded. `totalLength` is required and must match the declared length of the array attribute. Bare-name array items -(`Area001.Pump001.Speed`) are auto-normalized to the `[]` form at `AddItem` so -the array attribute accepts the write. +(`Area001.Pump001.Speed`) are auto-normalized to the `[]` form across the whole +add family — `AddItem`, `AddItem2`, `AddItemBulk`, and `AddBufferedItem` — so the +array attribute accepts the write. ## Galaxy Repository Browse @@ -396,7 +397,7 @@ repositories { } dependencies { - implementation 'com.zb.mom.ww.mxgateway:zb-mom-ww-mxgateway-client:0.1.1' + implementation 'com.zb.mom.ww.mxgateway:zb-mom-ww-mxgateway-client:0.1.2' } ```` diff --git a/clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java b/clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java index 6c31a5e..ac5ed22 100644 --- a/clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java +++ b/clients/java/zb-mom-ww-mxgateway-cli/src/test/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCliTests.java @@ -56,7 +56,7 @@ final class MxGatewayCliTests { assertEquals(0, run.exitCode()); assertEquals("", run.errors()); - assertTrue(run.output().contains("mxgateway-java 0.1.1")); + assertTrue(run.output().contains("mxgateway-java 0.1.2")); assertTrue(run.output().contains("gatewayProtocolVersion=3")); assertTrue(run.output().contains("workerProtocolVersion=1")); } @@ -86,7 +86,7 @@ final class MxGatewayCliTests { CliRun run = execute(new FakeClientFactory(), "version", "--json"); assertEquals(0, run.exitCode()); - assertTrue(run.output().contains("\"clientVersion\":\"0.1.1\"")); + assertTrue(run.output().contains("\"clientVersion\":\"0.1.2\"")); assertTrue(run.output().contains("\"gatewayProtocolVersion\":3")); } @@ -148,6 +148,26 @@ final class MxGatewayCliTests { assertTrue(run.output().contains("\"itemHandle\":7")); } + @Test + void adviseSupervisoryCommandCallsAdviseSupervisoryRaw() { + // Client.Java-050: dedicated test for advise-supervisory, using a + // separate adviseSupervisoryCalled flag so it cannot be masked by the + // plain advise path that shares adviseCalled. + FakeClientFactory factory = new FakeClientFactory(); + CliRun run = execute( + factory, + "advise-supervisory", + "--session-id", "session-cli", + "--server-handle", "12", + "--item-handle", "34", + "--json"); + + assertEquals(0, run.exitCode()); + assertTrue(factory.client.session.adviseSupervisoryCalled); + assertFalse(factory.client.session.adviseCalled, "plain advise must not be called"); + assertTrue(run.output().contains("\"kind\":\"MX_COMMAND_KIND_ADVISE_SUPERVISORY\"")); + } + // ---- ping subcommand (D4) ---- @Test @@ -1235,6 +1255,7 @@ final class MxGatewayCliTests { private boolean registerCalled; private boolean addItemCalled; private boolean adviseCalled; + private boolean adviseSupervisoryCalled; private MxValue lastWriteValue; private String lastPingMessage; private long lastReadBulkTimeoutMs; @@ -1304,7 +1325,7 @@ final class MxGatewayCliTests { @Override public MxCommandReply adviseSupervisoryRaw(int serverHandle, int itemHandle) { - adviseCalled = true; + adviseSupervisoryCalled = true; return MxCommandReply.newBuilder() .setKind(MxCommandKind.MX_COMMAND_KIND_ADVISE_SUPERVISORY) .setProtocolStatus(ok()) diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientVersion.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientVersion.java index d063fc5..8dbb6f7 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientVersion.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientVersion.java @@ -9,7 +9,7 @@ package com.zb.mom.ww.mxgateway.client; public final class MxGatewayClientVersion { private static final int GATEWAY_PROTOCOL_VERSION = 3; private static final int WORKER_PROTOCOL_VERSION = 1; - private static final String CLIENT_VERSION = "0.1.1"; + private static final String CLIENT_VERSION = "0.1.2"; private MxGatewayClientVersion() { } diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java index 7374981..8dd00e1 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java @@ -623,13 +623,22 @@ public final class MxGatewaySession implements AutoCloseable { * supplied indices must be within {@code [0, totalLength)}. Elements are iterated in * ascending index order so the produced command is deterministic. * + *

Because the proto fields {@code MxSparseArray.total_length} and + * {@code MxSparseElement.index} are {@code uint32}, passing a negative Java {@code int} + * would silently sign-extend to a large unsigned value on the wire. This method + * therefore rejects negative {@code totalLength} and negative element indices with + * {@link IllegalArgumentException} rather than allowing a hard-to-diagnose gateway error. + * * @param serverHandle the {@code ServerHandle} owning the item * @param itemHandle the {@code ItemHandle} to write * @param elementDataType the {@link MxDataType} of the array's elements - * @param totalLength the total length of the expanded array - * @param elements the indices to write mapped to their scalar values; unmentioned - * indices are reset to the element type default + * @param totalLength the total length of the expanded array; must be > 0 + * @param elements the indices to write mapped to their scalar values; each index must + * be in {@code [0, totalLength)}; unmentioned indices are reset to the element + * type default * @param userId the MXAccess user id used for security checks + * @throws IllegalArgumentException if {@code totalLength} is not positive, or if any + * element index is negative or ≥ {@code totalLength} * @throws MxGatewayException on transport or protocol failure */ public void writeArrayElements( @@ -641,6 +650,16 @@ public final class MxGatewaySession implements AutoCloseable { int userId) { Objects.requireNonNull(elementDataType, "elementDataType"); Objects.requireNonNull(elements, "elements"); + if (totalLength <= 0) { + throw new IllegalArgumentException("totalLength must be > 0, got " + totalLength); + } + for (Map.Entry entry : elements.entrySet()) { + int idx = entry.getKey(); + if (idx < 0 || idx >= totalLength) { + throw new IllegalArgumentException( + "element index " + idx + " is out of range [0, " + totalLength + ")"); + } + } MxSparseArray.Builder sparse = MxSparseArray.newBuilder() .setElementDataType(elementDataType) .setTotalLength(totalLength); diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java index 06be396..16cc720 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java @@ -451,6 +451,64 @@ final class MxGatewayClientSessionTests { } } + @Test + void writeArrayElementsRejectsNonPositiveTotalLength() throws Exception { + // Client.Java-051: negative/zero totalLength silently sign-extends to a + // large uint32 on the wire; the client must reject it with + // IllegalArgumentException before building the proto message (before any + // network call is issued). + try (InProcessGateway gateway = InProcessGateway.start( + new TestGatewayService() {}, new AtomicReference<>()); + MxGatewayClient client = gateway.client("", Duration.ofSeconds(5))) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "guard-session"); + + assertThrows( + IllegalArgumentException.class, + () -> session.writeArrayElements( + 1, 2, MxDataType.MX_DATA_TYPE_INTEGER, -1, Map.of(), 0), + "negative totalLength must throw"); + + assertThrows( + IllegalArgumentException.class, + () -> session.writeArrayElements( + 1, 2, MxDataType.MX_DATA_TYPE_INTEGER, 0, Map.of(), 0), + "zero totalLength must throw"); + } + } + + @Test + void writeArrayElementsRejectsOutOfRangeIndex() throws Exception { + // Client.Java-051: a negative index silently sign-extends to a large + // uint32 on the wire; an index >= totalLength exceeds the declared + // array bounds. Both must be caught before the network call. + try (InProcessGateway gateway = InProcessGateway.start( + new TestGatewayService() {}, new AtomicReference<>()); + MxGatewayClient client = gateway.client("", Duration.ofSeconds(5))) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "guard-session"); + + assertThrows( + IllegalArgumentException.class, + () -> session.writeArrayElements( + 1, 2, MxDataType.MX_DATA_TYPE_INTEGER, 5, + Map.of(-1, MxValues.int32Value(7)), 0), + "negative index must throw"); + + assertThrows( + IllegalArgumentException.class, + () -> session.writeArrayElements( + 1, 2, MxDataType.MX_DATA_TYPE_INTEGER, 5, + Map.of(5, MxValues.int32Value(7)), 0), + "index equal to totalLength must throw"); + + assertThrows( + IllegalArgumentException.class, + () -> session.writeArrayElements( + 1, 2, MxDataType.MX_DATA_TYPE_INTEGER, 5, + Map.of(10, MxValues.int32Value(7)), 0), + "index above totalLength must throw"); + } + } + private static ProtocolStatus ok() { return ProtocolStatus.newBuilder() .setCode(ProtocolStatusCode.PROTOCOL_STATUS_CODE_OK) diff --git a/clients/python/README.md b/clients/python/README.md index d8cdb80..3833809 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -170,8 +170,9 @@ await session.write_array_elements( ``` Bare-name array items (e.g. `Object.ArrayAttr` without an index suffix) added -via `add_item` auto-normalize to `[]` — they refer to the whole array, not a -single element. Writes through such handles must cover the full array or use +via `add_item`, `add_item2`, `add_item_bulk`, or `add_buffered_item` +auto-normalize to `[]` — they refer to the whole array, not a single element. +Writes through such handles must cover the full array or use `write_array_elements` to supply `total_length` and let the gateway fill defaults for the rest. diff --git a/clients/python/pyproject.toml b/clients/python/pyproject.toml index 366f434..bd09fbb 100644 --- a/clients/python/pyproject.toml +++ b/clients/python/pyproject.toml @@ -7,7 +7,7 @@ build-backend = "setuptools.build_meta" [project] name = "zb-mom-ww-mxaccess-gateway-client" version = "0.1.2" -description = "Async Python client scaffold for MXAccess Gateway." +description = "Async Python client for MXAccess Gateway." readme = "README.md" requires-python = ">=3.12" dependencies = [ diff --git a/clients/python/tests/test_review_findings_037_038.py b/clients/python/tests/test_review_findings_037_038.py new file mode 100644 index 0000000..937bb8e --- /dev/null +++ b/clients/python/tests/test_review_findings_037_038.py @@ -0,0 +1,163 @@ +"""Regression tests for Client.Python-037 and Client.Python-038. + +Client.Python-037: ``pyproject.toml`` description must not contain "scaffold". +Client.Python-038: ``advise-supervisory`` CLI subcommand must have coverage + (registration smoke test + happy-path command-shape test). + +Tests are TDD-first — written before the fix and expected to pass once the +source change lands. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any + +from click.testing import CliRunner + +from zb_mom_ww_mxgateway import ClientOptions, GatewayClient +from zb_mom_ww_mxgateway.generated import mxaccess_gateway_pb2 as pb +from zb_mom_ww_mxgateway_cli.commands import main + + +# --------------------------------------------------------------------------- +# Client.Python-037 — pyproject.toml description must not contain "scaffold". +# --------------------------------------------------------------------------- + + +def test_pyproject_description_does_not_contain_scaffold() -> None: + """The ``description`` field in ``pyproject.toml`` must not include the + word "scaffold" — a regression of Client.Python-001 that re-entered the + file at the package-rename commit. + """ + + pyproject = ( + Path(__file__).resolve().parent.parent / "pyproject.toml" + ).read_text(encoding="utf-8") + + # Find the description line and assert "scaffold" is absent. + for line in pyproject.splitlines(): + stripped = line.strip() + if stripped.startswith("description"): + assert "scaffold" not in stripped.lower(), ( + f"pyproject.toml description must not contain 'scaffold': {stripped!r}" + ) + return + + raise AssertionError("pyproject.toml has no 'description' line") + + +# --------------------------------------------------------------------------- +# Client.Python-038 — advise-supervisory must be registered + have a happy path. +# --------------------------------------------------------------------------- + + +def test_advise_supervisory_is_registered() -> None: + """``advise-supervisory`` must be a registered subcommand of ``main``. + + A ``--help`` invocation must exit 0 and the help text must include the + required options (--server-handle and --item-handle). + """ + + runner = CliRunner() + result = runner.invoke(main, ["advise-supervisory", "--help"]) + + assert result.exit_code == 0, result.output + assert "--server-handle" in result.output + assert "--item-handle" in result.output + + +# --------------- fake-stub infrastructure (mirrors test_review_findings_022_to_026) ---- + + +class _FakeUnary: + def __init__(self, replies: list[Any]) -> None: + self.replies = replies + self.requests: list[Any] = [] + self.metadata: tuple[tuple[str, str], ...] | None = None + + async def __call__(self, request: Any, *, metadata: tuple[tuple[str, str], ...]) -> Any: + self.requests.append(request) + self.metadata = metadata + return self.replies.pop(0) + + +class _FakeStub: + def __init__(self) -> None: + self.open_session = _FakeUnary( + [ + pb.OpenSessionReply( + session_id="session-1", + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + ), + ], + ) + self.invoke = _FakeUnary([]) + self.OpenSession = self.open_session + self.Invoke = self.invoke + + def set_invoke_replies(self, replies: list[Any]) -> None: + self.invoke.replies = replies + + +def _install_fake_connect(monkeypatch: Any, stub: _FakeStub) -> None: + """Patch ``GatewayClient.connect`` so the CLI uses the supplied fake stub.""" + + real_connect = GatewayClient.connect + + @classmethod # type: ignore[misc] + async def _spy_connect(cls: Any, options: ClientOptions, **kwargs: Any) -> GatewayClient: + return await real_connect(options, stub=stub) + + monkeypatch.setattr(GatewayClient, "connect", _spy_connect) + + +def test_cli_advise_supervisory_happy_path(monkeypatch: Any) -> None: + """``advise-supervisory`` must forward server_handle and item_handle in an + ``MX_COMMAND_KIND_ADVISE_SUPERVISORY`` ``MxCommand``. + + Pattern mirrors ``test_cli_acknowledge_alarm_happy_path`` in + ``test_review_findings_022_to_026.py``. + """ + + stub = _FakeStub() + stub.set_invoke_replies( + [ + pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_ADVISE_SUPERVISORY, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + ), + ], + ) + _install_fake_connect(monkeypatch, stub) + + runner = CliRunner() + result = runner.invoke( + main, + [ + "advise-supervisory", + "--endpoint", + "localhost:5000", + "--plaintext", + "--session-id", + "session-1", + "--server-handle", + "7", + "--item-handle", + "42", + "--json", + ], + ) + + assert result.exit_code == 0, result.output + payload = json.loads(result.output) + assert payload["ok"] is True + + # Verify the MxCommand shape forwarded to the gateway. + assert len(stub.invoke.requests) == 1 + cmd = stub.invoke.requests[0].command + assert cmd.kind == pb.MX_COMMAND_KIND_ADVISE_SUPERVISORY + assert cmd.advise_supervisory.server_handle == 7 + assert cmd.advise_supervisory.item_handle == 42 diff --git a/clients/rust/README.md b/clients/rust/README.md index 75a27e1..e01e645 100644 --- a/clients/rust/README.md +++ b/clients/rust/README.md @@ -196,10 +196,10 @@ the existing attribute value. #### Bare-name array AddItem normalisation -`AddItem` for a bare array attribute name (e.g. `Tank01.Temperature`) is -automatically normalised to `Tank01.Temperature[]` by the gateway so the -worker can resolve the full array. You do not need to append `[]` in client -code; the gateway handles it. +Adding a bare array attribute name (e.g. `Tank01.Temperature`) via `AddItem`, +`AddItem2`, `AddItemBulk`, or `AddBufferedItem` is automatically normalised to +`Tank01.Temperature[]` by the gateway so the worker can resolve the full array. +You do not need to append `[]` in client code; the gateway handles it. ## Galaxy Repository browse diff --git a/clients/rust/RustClientDesign.md b/clients/rust/RustClientDesign.md index 379bce8..13056cd 100644 --- a/clients/rust/RustClientDesign.md +++ b/clients/rust/RustClientDesign.md @@ -121,6 +121,7 @@ impl Session { pub async fn read_bulk>(&self, server_handle: i32, tag_addresses: &[S], timeout_ms: u32) -> Result, Error>; pub async fn write(&self, server_handle: i32, item_handle: i32, value: MxValue, user_id: i32) -> Result<(), Error>; pub async fn write2(&self, server_handle: i32, item_handle: i32, value: MxValue, timestamp_value: MxValue, user_id: i32) -> Result<(), Error>; + pub async fn write_array_elements(&self, server_handle: i32, item_handle: i32, element_data_type: MxDataType, total_length: u32, elements: impl IntoIterator, user_id: i32) -> Result<(), Error>; pub async fn write_bulk(&self, server_handle: i32, entries: Vec) -> Result, Error>; pub async fn write2_bulk(&self, server_handle: i32, entries: Vec) -> Result, Error>; pub async fn write_secured_bulk(&self, server_handle: i32, entries: Vec) -> Result, Error>; @@ -333,6 +334,7 @@ mxgw close-session --session-id mxgw register --session-id mxgw add-item --session-id --server-handle --item mxgw advise --session-id --server-handle --item-handle +mxgw advise-supervisory --session-id --server-handle --item-handle mxgw subscribe-bulk --session-id --server-handle --items mxgw unsubscribe-bulk --session-id --server-handle --item-handles mxgw read-bulk --session-id --server-handle --items [--timeout-ms ] diff --git a/clients/rust/tests/client_behavior.rs b/clients/rust/tests/client_behavior.rs index ab53e07..ae3f3d7 100644 --- a/clients/rust/tests/client_behavior.rs +++ b/clients/rust/tests/client_behavior.rs @@ -1212,7 +1212,9 @@ fn sparse_int32_value( .collect(); MxValue { - data_type: MxDataType::Integer as i32, + // outer data_type must be 0 (Unspecified); the element type lives only + // inside MxSparseArray.element_data_type, matching the + // `..ProtoMxValue::default()` used in Session::write_array_elements. variant_type: String::new(), kind: Some(Kind::SparseArrayValue(MxSparseArray { element_data_type: MxDataType::Integer as i32, @@ -1227,6 +1229,11 @@ fn sparse_int32_value( fn write_array_elements_proto_shape_has_sparse_oneof_kind() { let proto = sparse_int32_value(5, [(0, 10), (3, 30)]); + assert_eq!( + proto.data_type, 0, + "outer MxValue.data_type must be 0 (Unspecified); element type lives in element_data_type" + ); + let Kind::SparseArrayValue(ref sparse) = proto.kind.as_ref().unwrap() else { panic!("expected SparseArrayValue kind, got {:?}", proto.kind); }; @@ -1253,6 +1260,10 @@ fn write_array_elements_proto_shape_has_sparse_oneof_kind() { #[test] fn write_array_elements_empty_elements_is_valid_all_defaults() { let proto = sparse_int32_value(8, []); + assert_eq!( + proto.data_type, 0, + "outer MxValue.data_type must be 0 (Unspecified) even with no elements" + ); let Kind::SparseArrayValue(ref sparse) = proto.kind.as_ref().unwrap() else { panic!("expected SparseArrayValue kind"); }; @@ -1269,6 +1280,10 @@ fn sparse_array_value_round_trips_through_client_mx_value_projection_as_unset() // (e.g. a future version bug), the projection should degrade to Unset // rather than panic, because the enum variant is not readable. let proto = sparse_int32_value(4, [(1, 99)]); + assert_eq!( + proto.data_type, 0, + "outer MxValue.data_type must be 0 (Unspecified)" + ); let client_value = ClientMxValue::from_proto(proto); assert_eq!( client_value.projection(), diff --git a/code-reviews/Client.Dotnet/findings.md b/code-reviews/Client.Dotnet/findings.md index e2b2305..c773d85 100644 --- a/code-reviews/Client.Dotnet/findings.md +++ b/code-reviews/Client.Dotnet/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 1 | +| Open findings | 0 | ## Checklist coverage @@ -714,7 +714,7 @@ Re-review of the .NET client delta: `LazyBrowseNode` lazy paging + tests, the ne | Severity | Medium | | Category | Correctness & logic bugs | | Location | `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:91-93,113,2023-2050` | -| Status | Open | +| Status | Resolved | **Description:** `advise-supervisory` was added to the `command switch` dispatch table at line 113 but was not added to `IsKnownGatewayCommand` (the exhaustive list at lines 2023–2050). The guard at line 91 evaluates `IsKnownGatewayCommand(command)` before the dispatch table is reached; because `"advise-supervisory"` is absent from that list, `WriteUnknownCommand` is called and the method returns exit code 2 with "Unknown command 'advise-supervisory'." printed to stderr. The handler at line 113 is dead code — it can never execute. @@ -724,4 +724,4 @@ Note: `"advise"` is correctly present in `IsKnownGatewayCommand` (line 2030); th **Recommendation:** Add `or "advise-supervisory"` to the `IsKnownGatewayCommand` expression (e.g. after `"advise"` at line 2030). Add a test (`MxGatewayClientCliTests`) that invokes `advise-supervisory` through `RunAsync` with a fake client and asserts exit code 0 (not 2) and that the reply is written to stdout — this would have caught the regression immediately. -**Resolution:** _(empty until closed)_ +**Resolution:** 2026-06-18 — Confirmed root cause: `"advise-supervisory"` was absent from the `IsKnownGatewayCommand` expression in `MxGatewayClientCli.cs`, so the guard at line 91 intercepted every invocation and returned exit 2 "Unknown command 'advise-supervisory'." before the dispatch table was reached. Added `or "advise-supervisory"` after `or "advise"` at line 2031 in `IsKnownGatewayCommand`. Regression test `MxGatewayClientCliTests.RunAsync_AdviseSupervisory_IsRecognizedAndReachesDispatch` verified red (stderr contained "Unknown command 'advise-supervisory'.") against the pre-fix code and green after; full 86-test suite passes. diff --git a/code-reviews/Client.Go/findings.md b/code-reviews/Client.Go/findings.md index a15a747..acc10b4 100644 --- a/code-reviews/Client.Go/findings.md +++ b/code-reviews/Client.Go/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -825,7 +825,7 @@ Re-review of `clients/go/` changes since `8df5ab3`: `WriteArrayElements` default | Severity | Low | | Category | Code organization & conventions | | Location | `clients/go/cmd/mxgw-go/main.go:1298`, `clients/go/README.md:328-355` | -| Status | Open | +| Status | Resolved | **Description:** `advise-supervisory` is wired into the `run()` dispatch switch at `main.go:91-92` but is absent from two surfaces a user consults to discover CLI commands: @@ -836,6 +836,8 @@ This is exactly the shape Client.Go-012 (resolved 2026-05-20) and Client.Go-034 **Recommendation:** Add `advise-supervisory` to the `writeUsage` string and to the README subcommand table (e.g., `| advise-supervisory | Advise one item supervisory — required before a userID-attributed plain Write. |`). +**Resolution:** 2026-06-18 — Confirmed both omissions. Added `advise-supervisory` to the pipe-separated command list in `writeUsage()` (after `advise`) and added a row for it in the README "Subcommand reference" table: `| advise-supervisory | Advise one item supervisory — required before a user-id-attributed plain write. |`. `go build ./...` and `go test ./...` green. + ### Client.Go-036 | Field | Value | @@ -843,12 +845,14 @@ This is exactly the shape Client.Go-012 (resolved 2026-05-20) and Client.Go-034 | Severity | Low | | Category | Testing coverage | | Location | `clients/go/cmd/mxgw-go/main_test.go`, `clients/go/cmd/mxgw-go/main.go:364-399` | -| Status | Open | +| Status | Resolved | **Description:** `runAdviseSupervisory` has no CLI-level test in `main_test.go`. In particular the session-id-required guard at `main.go:376-378` is untested, unlike every other guard for session-id-required commands (e.g. `TestRunStreamEventsRequiresSessionID`, added in the same commit range by Client.Go-033). A future refactor that removes or conditions the guard has no regression catch. The pattern for adding such a test is already established in the test file and requires no bufconn fake. **Recommendation:** Add `TestRunAdviseSupervisoryRequiresSessionID` to `cmd/mxgw-go/main_test.go`, driving `runWithIO` with `[]string{"advise-supervisory", "-plaintext", "-api-key", "test"}` (no `-session-id`) and asserting `err.Error()` contains `"session-id is required"`. Mirrors `TestRunStreamEventsRequiresSessionID`. +**Resolution:** 2026-06-18 — Confirmed guard exists at `main.go:376-378` but had no test. Added `TestRunAdviseSupervisoryRequiresSessionID` to `cmd/mxgw-go/main_test.go` (mirrors `TestRunStreamEventsRequiresSessionID`): drives `runWithIO` with `["advise-supervisory", "-plaintext", "-api-key", "test"]` and asserts `err` contains `"session-id is required"`. Test passes immediately (guard was already present); pinned against future removal. `go test ./...` green. + ### Client.Go-037 | Field | Value | @@ -856,7 +860,7 @@ This is exactly the shape Client.Go-012 (resolved 2026-05-20) and Client.Go-034 | Severity | Low | | Category | Documentation & comments | | Location | `clients/go/README.md:136-137` | -| Status | Open | +| Status | Resolved | **Description:** The README "Write Semantics" section states: @@ -865,3 +869,5 @@ This is exactly the shape Client.Go-012 (resolved 2026-05-20) and Client.Go-034 The Go CLI has no standalone `write2` command — only `write2-bulk`. The analogous statement in the Python, Rust, and .NET README is accurate because those CLIs do expose `write2` as a standalone subcommand. A Go caller following this doc and attempting `mxgw-go write2 -session-id ... -type int32 -value 42 -timestamp 2026-01-01T00:00:00Z` receives `unknown command "write2"` (routed to the default branch of `run()`), not the expected MXAccess Write2 call. **Recommendation:** Change the sentence to accurately reflect the Go CLI surface, e.g.: "The CLI exposes the same command as `advise-supervisory`, and `write` takes `-user-id`." If a standalone `write2` command is intended for cross-client parity, add it (mirroring `runWrite` with the addition of a `-timestamp` flag and a `Write2Raw`/`Write2` SDK call). + +**Resolution:** 2026-06-18 — Confirmed root cause: Go CLI has no `write2` case in the `runWithIO` switch and no `runWrite2` function; only `write2-bulk` exists. Changed the sentence in `clients/go/README.md` from "The CLI exposes the same command as `advise-supervisory`, and `write` / `write2` take `--user-id`." to "The CLI exposes the same command as `advise-supervisory`, and `write` takes `-user-id`." No standalone `write2` command added (cross-client parity decision deferred to a future change). `go build ./...` and `go test ./...` green. diff --git a/code-reviews/Client.Java/findings.md b/code-reviews/Client.Java/findings.md index 389afca..7360df8 100644 --- a/code-reviews/Client.Java/findings.md +++ b/code-reviews/Client.Java/findings.md @@ -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 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 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) diff --git a/code-reviews/Client.Python/findings.md b/code-reviews/Client.Python/findings.md index 731196c..b2ed677 100644 --- a/code-reviews/Client.Python/findings.md +++ b/code-reviews/Client.Python/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -1566,7 +1566,7 @@ passed, 1 skipped, 0 warnings; previously 1 warning). The `tls`-marked | Severity | Low | | Category | Correctness & logic bugs | | Location | `clients/python/pyproject.toml:10` | -| Status | Open | +| Status | Resolved | **Description:** The `description` field in `pyproject.toml` reads `"Async Python client scaffold for MXAccess Gateway."` at commit `88915c3`. Client.Python-001 resolved this on 2026-05-18 by removing the word "scaffold". The fix was lost when commit `397d3c5` (the package directory rename, `src/mxgateway` → `src/zb_mom_ww_mxgateway`) re-created `pyproject.toml` from scratch, re-introducing the stale wording. The version bump commit `88915c3` carried the regression forward without correcting it. @@ -1574,6 +1574,8 @@ The issue is purely cosmetic and does not affect the wheel build or runtime beha **Recommendation:** Change the `description` in `clients/python/pyproject.toml` from `"Async Python client scaffold for MXAccess Gateway."` to `"Async Python client for MXAccess Gateway."` (drop "scaffold"), matching the fix applied under Client.Python-001. The `test_pip_wheel_build_succeeds` test will confirm the wheel still builds; no additional test is needed for a pure metadata word change. +**Resolution:** 2026-06-18 — Root cause confirmed: `pyproject.toml` line 10 still contained "scaffold" at commit `88915c3`. Removed "scaffold" from the `description` field so it now reads `"Async Python client for MXAccess Gateway."`, matching the Client.Python-001 fix and the sibling client descriptions. Added `test_pyproject_description_does_not_contain_scaffold` in `tests/test_review_findings_037_038.py` to prevent future regressions; the test failed before the fix and passes after. Full suite: 127 passed, 1 skipped. Generated directory unchanged. + ### Client.Python-038 | Field | Value | @@ -1581,7 +1583,7 @@ The issue is purely cosmetic and does not affect the wheel build or runtime beha | Severity | Low | | Category | Testing coverage | | Location | `clients/python/tests/`, `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:280-299,742-758` | -| Status | Open | +| Status | Resolved | **Description:** The new `advise-supervisory` CLI subcommand (commit `88915c3`) has no test coverage — not even a `--help` smoke registration test of the kind added for `stream-alarms` (`test_stream_alarms_is_registered`) or the earlier `advise` command. There is no test that: @@ -1596,3 +1598,5 @@ The pattern to follow is `test_cli_acknowledge_alarm_happy_path` in `tests/test_ 1. `test_advise_supervisory_is_registered` — `CliRunner().invoke(main, ["advise-supervisory", "--help"])` asserts exit code 0 and "AdviseSupervisory" (or the help text) is present. 2. `test_cli_advise_supervisory_happy_path` — injects a fake stub via `monkeypatch`, drives `advise-supervisory --session-id s1 --server-handle 1 --item-handle 2 --json`, and asserts the captured `MxCommand.kind == MX_COMMAND_KIND_ADVISE_SUPERVISORY`, `advise_supervisory.server_handle == 1`, `advise_supervisory.item_handle == 2`. + +**Resolution:** 2026-06-18 — Root cause confirmed: no test existed for `advise-supervisory` despite it being registered and implemented at commit `88915c3`. Added `tests/test_review_findings_037_038.py` with three tests: `test_advise_supervisory_is_registered` (CliRunner `--help` round-trip asserting exit 0 and `--server-handle`/`--item-handle` in output) and `test_cli_advise_supervisory_happy_path` (monkeypatched `GatewayClient.connect` with a fake stub, drives the CLI end-to-end, asserts `MxCommand.kind == MX_COMMAND_KIND_ADVISE_SUPERVISORY` and `advise_supervisory.server_handle == 7`, `advise_supervisory.item_handle == 42`). No source change was required — the command implementation was correct. Full suite: 127 passed, 1 skipped. Generated directory unchanged. diff --git a/code-reviews/Client.Rust/findings.md b/code-reviews/Client.Rust/findings.md index 6022cb6..2a2dc3d 100644 --- a/code-reviews/Client.Rust/findings.md +++ b/code-reviews/Client.Rust/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-06-18 | | Commit reviewed | `88915c3` | | Status | Re-reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -898,7 +898,7 @@ This is masked by the tests: `tls_with_require_certificate_validation_does_not_s | Severity | Low | | Category | Design-document adherence | | Location | `clients/rust/RustClientDesign.md:101-131` (Session API block); `clients/rust/RustClientDesign.md:326-353` (CLI commands table) | -| Status | Open | +| Status | Resolved | **Description:** The diff adds two pieces of new public surface that are not reflected in `RustClientDesign.md`: @@ -924,6 +924,8 @@ pub async fn write_array_elements( Add a sentence noting that the `elements` iterator accepts `(index, value)` pairs (not a `HashMap`, so duplicate indices are forwarded to the gateway, which rejects them with `InvalidArgument`). Add `mxgw advise-supervisory --session-id --server-handle --item-handle ` to the CLI table. +**Resolution:** 2026-06-18 — Added `write_array_elements` (with its exact `session.rs` signature) to the Session API block in `RustClientDesign.md` between `write2` and `write_bulk`. Added `mxgw advise-supervisory --session-id --server-handle --item-handle ` to the CLI commands table after `mxgw advise`. Both signatures verified against `clients/rust/src/session.rs:567` and `clients/rust/crates/mxgw-cli/src/main.rs:109-120`. `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, and `cargo test --workspace` all pass. + ### Client.Rust-040 | Field | Value | @@ -931,7 +933,7 @@ Add a sentence noting that the `elements` iterator accepts `(index, value)` pair | Severity | Low | | Category | Testing coverage | | Location | `clients/rust/tests/client_behavior.rs:1195-1224` (`sparse_int32_value` helper); `clients/rust/tests/client_behavior.rs:1226-1264` (unit tests using the helper) | -| Status | Open | +| Status | Resolved | **Description:** The `sparse_int32_value` test helper (lines 1195-1224) carries this comment: "Build the proto `MxValue` that `write_array_elements` would send." It then constructs the outer `MxValue` with `data_type: MxDataType::Integer as i32` (line 1215). However, `write_array_elements` in `session.rs` uses `..ProtoMxValue::default()` for the outer value, which sets `data_type` to `0` (= `MxDataType::Unspecified`). The helper builds the old, incorrect shape that the known bug fix (`72cf2f4`) explicitly corrected — the outer `data_type` should carry the element type only inside `SparseArray.element_data_type`, not on the enclosing `MxValue`. @@ -940,3 +942,5 @@ The two unit tests that use this helper (`write_array_elements_proto_shape_has_s If the `..ProtoMxValue::default()` line were ever accidentally changed back to set `data_type` from `element_data_type`, the unit tests would continue to pass while the e2e test would catch the regression — but the test comment explicitly says the helper represents "what `write_array_elements` would send," making the incorrect `data_type` in the helper actively misleading for future maintainers. **Recommendation:** Fix the `sparse_int32_value` helper to use `..MxValue::default()` (which zeros `data_type`) instead of `data_type: MxDataType::Integer as i32`, so the helper accurately represents the wire shape `write_array_elements` actually emits. Then add an explicit `assert_eq!(proto.data_type, 0, "outer MxValue.data_type must be Unspecified")` assertion to `write_array_elements_proto_shape_has_sparse_oneof_kind` so the unit test also locks in the outer-`data_type` fix — providing a second, faster regression guard that does not require spinning up a fake gRPC server. + +**Resolution:** 2026-06-18 — Root cause confirmed: `sparse_int32_value` set `data_type: MxDataType::Integer as i32` on the outer `MxValue`, contradicting the `..ProtoMxValue::default()` in `Session::write_array_elements` which leaves `data_type = 0`. Fixed the helper to use `..MxValue::default()` (removing the explicit `data_type` field), so the outer `MxValue.data_type` is now `0` (Unspecified), matching the actual wire shape. Added `assert_eq!(proto.data_type, 0, …)` assertions to all three unit tests that call the helper: `write_array_elements_proto_shape_has_sparse_oneof_kind`, `write_array_elements_empty_elements_is_valid_all_defaults`, and `sparse_array_value_round_trips_through_client_mx_value_projection_as_unset`. All 36 tests pass (`cargo test --workspace`); `cargo fmt --check` and `cargo clippy --workspace --all-targets -- -D warnings` are clean.