Resolve Client.Rust-022..029: MalformedReply, correlation ids, clippy
Client.Rust-022 Restored Error::MalformedReply for register / add_item /
add_item2 and the bulk-subscribe / read-bulk / write-bulk
dispatch arms so malformed-but-OK replies fail loudly
instead of returning Vec::new().
Client.Rust-023 Restored next_correlation_id and routed every CLI close /
stream-alarms / acknowledge-alarm / bench-read-bulk call
through it so each call carries a unique opaque token.
Client.Rust-024 Added round-trip tests for read_bulk / write_bulk /
write2_bulk / write_secured_bulk / write_secured2_bulk
plus stream_alarms and percentile_summary unit tests.
Client.Rust-025 RustClientDesign.md re-synced — new bulk SDK, alarms
surface, Error variants, CLI command list, and the
Windows stack workaround.
Client.Rust-026 Session::read_bulk now borrows a tag slice; bench-read-
bulk binds tags once outside the warm-up / steady-state
loops.
Client.Rust-027 .cargo/config.toml selector tightened to
cfg(all(windows, target_env = "msvc")) and comment
rewritten to match reality (release + debug ship the
8 MB reservation).
Client.Rust-028 run_batch removed the empty-line break; stdin EOF is
the only terminator.
Client.Rust-029 Re-applied Client.Rust-001 / 002 / 012 — added the
missing doc comments, renamed BulkReplyKind variants,
and replaced the clone-on-copy with a deref under lock
so cargo clippy -D warnings is clean.
All resolved at 2026-05-24; cargo fmt + check + clippy + test all green
(55 tests).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-24 |
|
||||
| Commit reviewed | `42b0037` |
|
||||
| Status | Re-reviewed |
|
||||
| Open findings | 8 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -481,7 +481,7 @@ The CLI integration in Client.Rust-014 works either way; this is solely about de
|
||||
| Severity | Medium |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `clients/rust/src/session.rs:369-391,403-420,427-444,452-469,476-493,631-696,706-724` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Commit `3251069` re-introduced the bulk read/write SDK methods (`read_bulk`, `write_bulk`, `write2_bulk`, `write_secured_bulk`, `write_secured2_bulk`) on `Session`. Each method falls back to `Vec::new()` when an OK reply does not carry the expected typed payload arm:
|
||||
|
||||
@@ -496,6 +496,8 @@ and `bulk_write_results` does the same for the four write families. A caller of
|
||||
|
||||
**Recommendation:** Re-apply the Client.Rust-005/006 resolutions on top of the new methods: add `Error::MalformedReply { detail: String }` back to `error.rs`, change `register_server_handle` / `add_item_handle` / `add_item2_handle` to return `Result<i32, Error>` (yielding `MalformedReply` when the reply lacks an extractable handle), change `bulk_results` and `bulk_write_results` to return `Result<Vec<_>, Error>` (yielding `MalformedReply` on a mismatched / absent payload arm), and route the same fix through the new `read_bulk` inline branch. Re-add the six `…_returns_malformed_reply_…` tests from the Client.Rust-016 resolution to lock the contract in.
|
||||
|
||||
**Resolution:** 2026-05-24 — Re-added `Error::MalformedReply { detail: String }` to `clients/rust/src/error.rs` (alongside re-adding `Error::Unavailable` for Client.Rust-010's resolution). Changed `register_server_handle` / `add_item_handle` / `add_item2_handle` in `clients/rust/src/session.rs` to return `Result<i32, Error>` and yield `MalformedReply` when the reply lacks both a typed payload and an int32 `return_value`. Changed `bulk_results` and `bulk_write_results` to return `Result<Vec<_>, Error>` and yield `MalformedReply` on a mismatched or absent payload arm. Rewrote `read_bulk`'s inline branch the same way. Added six regression tests in `clients/rust/tests/client_behavior.rs` — `register_returns_malformed_reply_when_ok_reply_has_no_payload`, `add_item_returns_malformed_reply_when_ok_reply_has_no_payload`, `add_item2_returns_malformed_reply_when_ok_reply_has_no_payload`, `subscribe_bulk_returns_malformed_reply_on_mismatched_payload_arm`, `read_bulk_returns_malformed_reply_on_mismatched_payload_arm`, `write_bulk_returns_malformed_reply_on_mismatched_payload_arm` — driven by a new `InvokeOverride` enum on `FakeState` that pins the fake gateway's `Invoke` handler to `OkWithoutPayload`, `OkWithMismatchedPayload`, or `Unavailable(...)` per test.
|
||||
|
||||
### Client.Rust-023
|
||||
|
||||
| Field | Value |
|
||||
@@ -503,7 +505,7 @@ and `bulk_write_results` does the same for the four write families. A caller of
|
||||
| Severity | Low |
|
||||
| Category | mxaccessgw conventions |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:835,872,1476` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Three CLI subcommands added since `d692232` hard-code their `client_correlation_id`:
|
||||
|
||||
@@ -517,6 +519,8 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
|
||||
**Recommendation:** Replace the three string literals with calls to the existing `next_correlation_id` helper (already `pub` and intended for raw-RPC consumers): `zb_mom_ww_mxgateway_client::session::next_correlation_id("cli-stream-alarms")`, `next_correlation_id("cli-acknowledge-alarm")`, `next_correlation_id("cli-bench-read-bulk-close")`. While here, restore the same fix at lines 506 and 553 so the CLI surface as a whole shares the library's correlation-id discipline.
|
||||
|
||||
**Resolution:** 2026-05-24 — Restored `pub fn next_correlation_id(label: &str) -> String` in `clients/rust/src/session.rs` (the `d692232` rename had reverted Client.Rust-011/014's resolution along with the rest) and re-exported it at the crate root in `clients/rust/src/lib.rs` (`pub use session::{next_correlation_id, Session};`) so the in-tree `mxgw` CLI calls the short `zb_mom_ww_mxgateway_client::next_correlation_id(...)` path. Replaced all five hard-coded correlation-id literals in `clients/rust/crates/mxgw-cli/src/main.rs` (`rust-cli-ping`, `rust-cli-close-session`, `rust-cli-stream-alarms`, `rust-cli-acknowledge-alarm`, `rust-cli-bench-read-bulk-close`) with `next_correlation_id("cli-...")`. `Session::command_request` and `Session::close` now use the same helper. Added a regression test `cli_subcommands_propagate_unique_correlation_ids_from_next_correlation_id` in `clients/rust/tests/client_behavior.rs` that records the correlation id observed at the fake gateway and asserts both that successive `next_correlation_id` calls produce distinct values and that the label is propagated through `stream_alarms` / `acknowledge_alarm`.
|
||||
|
||||
### Client.Rust-024
|
||||
|
||||
| Field | Value |
|
||||
@@ -524,7 +528,7 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
| Severity | Medium |
|
||||
| Category | Testing coverage |
|
||||
| Location | `clients/rust/tests/client_behavior.rs:405-415`; `clients/rust/src/session.rs:369-493`; `clients/rust/src/client.rs:265-291`; `clients/rust/crates/mxgw-cli/src/main.rs:1310-1505` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The diff under review adds substantial SDK and CLI surface with no positive-path coverage:
|
||||
|
||||
@@ -534,6 +538,8 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
|
||||
**Recommendation:** Extend the fake-gateway `Invoke` dispatcher in `tests/client_behavior.rs` with the five new bulk reply arms, add round-trip tests for each (`write_bulk` / `write2_bulk` / `write_secured_bulk` / `write_secured2_bulk` / `read_bulk`), and re-add the six malformed-reply tests from the Client.Rust-016 resolution. Replace the trivial `stream_alarms` stub with one that emits a synthetic `ActiveAlarm` → `SnapshotComplete` → `Transition` sequence and assert the client surfaces them in order. For the bench, factor the percentile / accounting helpers out of `run_bench_read_bulk` into a small struct (matching the previous `BenchReadBulkStats`) and add unit tests asserting `latencyMs.{p50,p95,p99,max,mean}` are computed correctly from a hand-built sample.
|
||||
|
||||
**Resolution:** 2026-05-24 — Extended the fake gateway's `Invoke` dispatcher to handle `Register`, `AddItem2`, `ReadBulk`, `WriteBulk`, `Write2Bulk`, `WriteSecuredBulk`, and `WriteSecured2Bulk`, with shared `write_bulk_reply_for` / `bulk_write_result_ok` helpers. Added round-trip integration tests in `clients/rust/tests/client_behavior.rs` for all five bulk SDK methods: `read_bulk_round_trips_through_the_fake_gateway`, `write_bulk_round_trips_through_the_fake_gateway`, `write2_bulk_round_trips_through_the_fake_gateway`, `write_secured_bulk_round_trips_through_the_fake_gateway`, `write_secured2_bulk_round_trips_through_the_fake_gateway`. Replaced the trivial `stream_alarms` stub with a per-test script (`FakeState::stream_alarms_script`) and added `stream_alarms_emits_snapshot_then_complete_then_transition_in_order`, which feeds an `ActiveAlarm` → `SnapshotComplete` → `Transition` sequence and asserts the client surfaces all three payload arms in order. Added three CLI-side unit tests against the existing `percentile_summary` helper in `clients/rust/crates/mxgw-cli/src/main.rs`: `bench_percentile_summary_matches_hand_built_sample` (asserts p50/p95/p99/max/mean on the sample `[1,2,3,4,5]`), `bench_percentile_summary_handles_empty_sample`, and `bench_percentile_summary_handles_single_value_sample`. The malformed-reply suite from Client.Rust-022 plus the unary `Error::Unavailable` test together cover the remaining surface.
|
||||
|
||||
### Client.Rust-025
|
||||
|
||||
| Field | Value |
|
||||
@@ -541,7 +547,7 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
| Severity | Low |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `clients/rust/RustClientDesign.md:92-106,142-153,164-171` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** CLAUDE.md mandates that "When public APIs, contracts, configuration, build steps, security behavior, event shapes, value conversion, status mapping, or lifecycle rules change, the affected docs ... must change in the same commit." The diff under review adds the following public surface, none of which is reflected in `RustClientDesign.md`:
|
||||
|
||||
@@ -553,6 +559,8 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
|
||||
**Recommendation:** Bring `RustClientDesign.md` back in sync with the actual implementation. Restore the `Session` API block to enumerate the five new bulk read/write methods alongside the existing six bulk-subscribe helpers (cross-reference Client.Rust-019's recommendation for the correct signatures — no trailing positional `user_id`/`timestamp`/`current_user_id`/`verifier_user_id`, those live on the per-entry structs). Add `stream_alarms` / `acknowledge_alarm` / `AlarmFeedStream` to a new "Alarms" section adjacent to the existing event-stream section. Expand the CLI command list to enumerate every subcommand the binary exposes today. Expand the `Error` enum sketch to match `clients/rust/src/error.rs`. Add a short "Windows build notes" subsection documenting the `.cargo/config.toml` stack workaround and why clap-derive's large `Command` enum needs it.
|
||||
|
||||
**Resolution:** 2026-05-24 — Brought `clients/rust/RustClientDesign.md` back in sync with the implementation. Removed the orphan `tracing` dependency line and added a new "Windows Build Notes" section explaining the `clients/rust/.cargo/config.toml` `/STACK:8388608` MSVC link-arg (why clap-derive's large `Command` enum needs it, that it writes into `IMAGE_OPTIONAL_HEADER.SizeOfStackReserve` for both debug and release builds, and that the MSVC-only target selector keeps mingw unaffected). Extended the `Session` API block to enumerate the five new bulk read/write helpers — `read_bulk<S: AsRef<str>>(&[S], u32)`, `write_bulk(Vec<WriteBulkEntry>)`, `write2_bulk(Vec<Write2BulkEntry>)`, `write_secured_bulk(Vec<WriteSecuredBulkEntry>)`, `write_secured2_bulk(Vec<WriteSecured2BulkEntry>)` — plus `add_item2`, `un_advise`, `remove_item`, and `write2`. Added a paragraph noting that per-entry credentials/timestamps live on the entry structs and that `read_bulk`'s borrow-and-`AsRef<str>` shape is what the cross-language bench-read-bulk hot loop relies on. Added a new "Alarms" section with `GatewayClient::stream_alarms` / `acknowledge_alarm` / the `AlarmFeedStream` type alias and the always-on (no-session) snapshot → complete → transition contract. Expanded the `Error` enum block to match `clients/rust/src/error.rs` (every public variant including `MalformedReply`, `Unavailable`, `InvalidEndpoint`, `InvalidArgument`). Expanded the CLI command list to enumerate every subcommand the binary exposes today, with notes on `batch`'s EOF-only termination and `bench-read-bulk`'s cross-language JSON shape.
|
||||
|
||||
### Client.Rust-026
|
||||
|
||||
| Field | Value |
|
||||
@@ -560,7 +568,7 @@ Every invocation of these subcommands on the same machine — and every iteratio
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:1402-1406,1419-1423` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `run_bench_read_bulk` clones the `tags: Vec<String>` on every iteration of both the warmup loop and the steady-state measurement loop:
|
||||
|
||||
@@ -586,6 +594,8 @@ Each clone allocates one fresh `Vec<String>` plus `bulk_size` heap-allocated `St
|
||||
|
||||
**Recommendation:** Change `Session::read_bulk` to take `tag_addresses: &[String]` or generic over `AsRef<str>` (as Client.Rust-019's recommendation noted is what `read_bulk` was at `a020350`) so the bench can pass `&tags` once and avoid the per-call clone. Alternatively keep the `Vec<String>` signature but borrow the underlying buffer for the hot loop — e.g. build the bench's payload once outside the steady-state window and re-use it. The current `read_bulk(..., Vec<String>, ...)` shape forces the clone at the call site.
|
||||
|
||||
**Resolution:** 2026-05-24 — Changed `Session::read_bulk` to `read_bulk<S: AsRef<str>>(&self, server_handle, tag_addresses: &[S], timeout_ms)` so the bench loop can borrow the tag list once instead of cloning it per iteration. `run_bench_read_bulk` now binds `let tags_ref: &[String] = &tags;` outside the warm-up / steady-state loops and passes `tags_ref` into both, eliminating the per-call `Vec<String>` + `String` clone-allocations that were previously charged into the per-call latency reported by the cross-language `latencyMs` JSON contract. The CLI `read-bulk` subcommand was updated to call `session.read_bulk(server_handle, &items, timeout_ms)`. Clippy's `clone_on_copy` / `redundant_clone` lints are clean at HEAD so no extra regression test is needed beyond `read_bulk_round_trips_through_the_fake_gateway` from Client.Rust-024.
|
||||
|
||||
### Client.Rust-027
|
||||
|
||||
| Field | Value |
|
||||
@@ -593,7 +603,7 @@ Each clone allocates one fresh `Vec<String>` plus `bulk_size` heap-allocated `St
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `clients/rust/.cargo/config.toml:1-9` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The new build-config file added by `71d2c39` carries this leading comment:
|
||||
|
||||
@@ -616,6 +626,8 @@ Two issues with the documentation vs the configuration:
|
||||
|
||||
**Recommendation:** Either tighten the `[target.…]` selector to only the MSVC-linker tier (`cfg(all(windows, target_env = "msvc"))`) and re-word the comment to make the release-build behaviour explicit ("the stack reservation goes into the PE header for both debug and release builds; release is unaffected at runtime because the optimizer elides the enum from the stack frame"), or add the `+gnu` variant as a parallel block using `-Wl,--stack,8388608`. As a complementary fix, see Client.Rust-024's recommendation — boxing the largest clap-derive variants would let the stack workaround be retired entirely.
|
||||
|
||||
**Resolution:** 2026-05-24 — Tightened the target selector in `clients/rust/.cargo/config.toml` from `cfg(windows)` to `cfg(all(windows, target_env = "msvc"))` so `x86_64-pc-windows-gnu` (mingw) builds, which route link args through the GNU linker and reject `/STACK:`, are no longer affected. Rewrote the leading comment to make the release-build behaviour explicit: the `/STACK:` link-arg writes into `IMAGE_OPTIONAL_HEADER.SizeOfStackReserve` at link time and applies to both debug and release builds (so release artifacts ship with the same 8 MB stack reservation); release builds do not need it at runtime because the optimizer elides the enum from the stack frame, but the setting is kept on so both flavours produce binaries with identical stack metadata. The doc note in `RustClientDesign.md` now mirrors this.
|
||||
|
||||
### Client.Rust-028
|
||||
|
||||
| Field | Value |
|
||||
@@ -623,7 +635,7 @@ Two issues with the documentation vs the configuration:
|
||||
| Severity | Low |
|
||||
| Category | mxaccessgw conventions |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:1126-1166` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `run_batch` reads commands from stdin with the blocking `std::io::Stdin::lock().lines()` iterator while the surrounding function is `async fn` and the runtime is `#[tokio::main]` (multi-threaded by default). Each `for line in stdin.lock().lines()` iteration pins one of tokio's worker threads on a blocking syscall (`ReadFile` on Windows), then spawns the dispatch as a separate `tokio::task::spawn(dispatch(cli.command))` — which itself runs on another worker thread — and awaits its `JoinHandle`. The pattern works for a single-threaded harness driver but has two latent problems:
|
||||
|
||||
@@ -632,6 +644,8 @@ Two issues with the documentation vs the configuration:
|
||||
|
||||
**Recommendation:** Replace the empty-line break with an EOF-only terminator (`if line.trim().is_empty() { println!("{BATCH_EOR}"); stdout.lock().flush().ok(); continue; }`) so accidental blank lines log an empty-EOR-bracketed result instead of ending the session. Optionally wrap the stdin reader in `tokio::task::spawn_blocking` so the runtime can move it onto a dedicated blocking-pool thread; the current shape works for today's harness contract but is brittle if the dispatch future ever needs to share the runtime with stdin reads.
|
||||
|
||||
**Resolution:** 2026-05-24 — Removed the `if line.is_empty() { break; }` empty-line sentinel in `run_batch` so the only terminator is now stdin EOF (the implicit end of the `for line in stdin.lock().lines()` iterator). Blank or whitespace-only lines now fall into the existing `parts.is_empty()` branch which logs `__MXGW_BATCH_EOR__` and continues, matching the other four language CLIs and shielding the PowerShell e2e harness from accidental `Write-Output ""` calls between commands. Added a comment block on `run_batch` explaining the tokio-runtime / blocking-stdin trade-off: dispatch is already spawned on a fresh tokio task so the blocking iterator parks at most one worker thread on `ReadFile`, and that is acceptable because no other future on the main task needs to run while we wait for the next command. The CLI doc comment on the `Batch` clap variant was updated to mirror the new semantics. No unit test added — `run_batch` reads from real stdin which can't be driven from a `#[tokio::test]` without a subprocess harness, and the existing `parses_batch_command` / `batch_eor_marker_is_stable` tests already cover the parser and the EOR sentinel.
|
||||
|
||||
### Client.Rust-029
|
||||
|
||||
| Field | Value |
|
||||
@@ -639,7 +653,7 @@ Two issues with the documentation vs the configuration:
|
||||
| Severity | High |
|
||||
| Category | mxaccessgw conventions |
|
||||
| Location | `clients/rust/src/options.rs:98,143`; `clients/rust/src/galaxy.rs:282`; `clients/rust/src/session.rs:664-671` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `cargo clippy --workspace --all-targets -- -D warnings` fails at HEAD `42b0037` with three errors that the prior d692232 reviewer noted as "out of scope for Client.Rust-021" but did not open as a tracked finding:
|
||||
|
||||
@@ -671,3 +685,5 @@ All three were resolved at `a020350` (Client.Rust-001, Client.Rust-002, Client.R
|
||||
The third error (`BulkReplyKind` enum-variant-names) is also touched by the diff under review: commit `3251069` added a sibling enum `BulkWriteReplyKind` at `session.rs:699-704` whose variants (`Write`, `Write2`, `WriteSecured`, `WriteSecured2`) do not share a suffix and so do not trip the lint — but the pattern is now duplicated. A fix should rename both enums consistently or apply the same scoped `#[allow(clippy::enum_variant_names)]` reason-annotated allow to both.
|
||||
|
||||
**Recommendation:** Re-apply Client.Rust-001 (add doc comments on `with_max_grpc_message_bytes` / `max_grpc_message_bytes` in `options.rs`), Client.Rust-002 (drop the `Bulk` suffix from `BulkReplyKind`'s variants so they become `AddItem` / `AdviseItem` / …, or add a narrowly-scoped `#[allow(clippy::enum_variant_names)]` with a reason comment), and Client.Rust-012 (replace `last_deploy.lock().unwrap().clone()` with `*last_deploy.lock().unwrap()` in `galaxy.rs:282`). Verify with `cargo clippy --workspace --all-targets -- -D warnings`. Consider adding a pre-commit / CI gate so the next reviewer never has to discover the regression by running clippy.
|
||||
|
||||
**Resolution:** 2026-05-24 — Re-applied all three resolutions. `clients/rust/src/options.rs` now has `///` doc comments on `with_max_grpc_message_bytes` and `max_grpc_message_bytes`. `clients/rust/src/galaxy.rs:282` uses `*self.state.last_deploy.lock().unwrap()` instead of `.clone()`. `clients/rust/src/session.rs`'s `BulkReplyKind` variants are renamed to `AddItem` / `AdviseItem` / `RemoveItem` / `UnAdviseItem` / `Subscribe` / `Unsubscribe` (no shared `Bulk` suffix), with the call sites in `add_item_bulk` / `advise_item_bulk` / `remove_item_bulk` / `un_advise_item_bulk` / `subscribe_bulk` / `unsubscribe_bulk` updated accordingly. The sibling `BulkWriteReplyKind` already had non-suffix-sharing variants (`Write` / `Write2` / `WriteSecured` / `WriteSecured2`) and required no rename. `cargo clippy --workspace --all-targets -- -D warnings` is clean at HEAD.
|
||||
|
||||
Reference in New Issue
Block a user