code-reviews: 2026-06-18 re-review of array-write-ergonomics feature at 88915c3
Re-reviewed the 10 modules touched by the MxSparseArray / array-write ergonomics work (8df5ab3..88915c3). 16 new findings: - Server-057 (Medium): [] AddItem normalization skips AddItemBulk/AddBufferedItem - Client.Dotnet-030 (Medium): advise-supervisory missing from IsKnownGatewayCommand (dead command) - 14 Low: MxSparseArray doc/test gaps, advise-supervisory CLI gaps across clients, Client.Java-049 / Client.Python-037 version-bump consistency misses Worker.Tests and IntegrationTests clean. Worker unchanged by the feature, not re-reviewed.
This commit is contained in:
Joseph Doherty
@@ -4,10 +4,10 @@
|
||||
|---|---|
|
||||
| Module | `clients/rust` |
|
||||
| Reviewer | Claude Code |
|
||||
| Review date | 2026-06-16 |
|
||||
| Commit reviewed | `8df5ab3` |
|
||||
| Review date | 2026-06-18 |
|
||||
| Commit reviewed | `88915c3` |
|
||||
| Status | Re-reviewed |
|
||||
| Open findings | 0 |
|
||||
| Open findings | 2 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -132,6 +132,27 @@ Re-review of the Rust client delta: options.rs TLS trust decision, mxgw-cli gala
|
||||
| 9 | Testing coverage | Client.Rust-038 |
|
||||
| 10 | Documentation & comments | No issues found |
|
||||
|
||||
### 2026-06-18 review (commit 88915c3)
|
||||
|
||||
Re-review of `git diff 8df5ab3..88915c3 -- clients/rust/`. The diff introduces: `Session::write_array_elements` sparse-array default-fill helper (`src/session.rs`); `SparseArrayValue` → `Unset` decode mapping in `MxValueProjection` (`src/value.rs`); `advise-supervisory` CLI subcommand (`crates/mxgw-cli/src/main.rs`); README and `RustClientDesign.md` docs additions; version bump 0.1.1 → 0.1.2; and a suite of tests in `tests/client_behavior.rs`.
|
||||
|
||||
Known prior bug (commit `72cf2f4`): `write_array_elements` set the outer `ProtoMxValue.data_type` to the element type — confirmed fixed via `..ProtoMxValue::default()` (outer `data_type = 0`). The e2e test `write_array_elements_routes_sparse_array_write_through_fake_gateway` asserts `value.data_type == 0` and `sparse.element_data_type == Integer`, correctly locking the fix in. The `MxValue` roundtrip is sound: `MxValue::from_proto(sparse_value).into_proto()` returns the original raw proto unchanged, so the sparse payload reaches the wire unmodified.
|
||||
|
||||
All prior findings 033–038 confirmed Resolved at `8df5ab3`. `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, and `cargo test --workspace` are assumed clean at HEAD (source review only; toolchain is Windows-only and not available on this macOS host).
|
||||
|
||||
| # | Category | Result |
|
||||
|---|---|---|
|
||||
| 1 | Correctness & logic bugs | Issue found (Client.Rust-040): the `sparse_int32_value` unit-test helper sets `data_type: MxDataType::Integer as i32` on the outer `MxValue` but `write_array_elements` uses `..ProtoMxValue::default()` (outer `data_type = 0`); the helper comment claims it builds "the proto MxValue that `write_array_elements` would send" so the unit tests using it test a subtly incorrect shape. The e2e test correctly covers the fix, but the unit test helper should match the implementation. |
|
||||
| 2 | mxaccessgw conventions | No issues found — `advise-supervisory` goes through `session.invoke` which calls `command_request`, which calls `next_correlation_id` internally; unique per-call correlation ids are preserved. `cargo fmt --check` and `cargo clippy --workspace --all-targets -- -D warnings` are expected clean (no new lint-tripping patterns introduced). |
|
||||
| 3 | Concurrency & thread safety | No issues found — `write_array_elements` is a thin synchronous builder that delegates to the existing `Session::write` async path; `last_write_command` in `FakeState` is behind a `Mutex<Option<_>>` and accessed correctly. No new `unsafe`, no new shared mutable state. |
|
||||
| 4 | Error handling & resilience | No issues found — `total_length = 0`, out-of-range indices, duplicate indices, and element-kind mismatches are all validated by the gateway's `SparseArrayExpander` and surface as `Error::InvalidArgument` (propagated, per the doc comment). No client-side guard is needed; the gateway is the single validation point. |
|
||||
| 5 | Security | No issues found — `write_array_elements` passes `user_id` through to `Session::write` which is already covered by the existing API-key + scope enforcement path; no credentials or secrets in the new surface. |
|
||||
| 6 | Performance & resource management | No issues found — `impl IntoIterator<Item = (u32, MxValue)>` avoids requiring an intermediate `Vec`; elements are collected once into `Vec<MxSparseElement>` and immediately handed to the proto. No unnecessary clones on the hot path. |
|
||||
| 7 | Design-document adherence | Issue found (Client.Rust-039): `Session::write_array_elements` is a new public SDK method and `advise-supervisory` is a new CLI subcommand; neither appears in `RustClientDesign.md` (Session API block or CLI commands table). CLAUDE.md requires docs to change in the same commit as the source. README was correctly updated. |
|
||||
| 8 | Code organization & conventions | No issues found — `register_page_token` was cleanly extracted from `browse_children_one_level` and covered with a unit test. `write_array_elements` is placed adjacent to `write` in `session.rs`. The `WriteOk` `InvokeOverride` variant and `last_write_command` capture are well-scoped to the test infrastructure. |
|
||||
| 9 | Testing coverage | Cross-referenced with Client.Rust-040: the `sparse_int32_value` test helper tests a proto shape with incorrect outer `data_type`; the unit tests using it do not verify `data_type` and would not catch a regression of the outer-`data_type` fix. The e2e test `write_array_elements_routes_sparse_array_write_through_fake_gateway` does assert `data_type == 0` and provides the real regression guard. |
|
||||
| 10 | Documentation & comments | No issues found — README `write_array_elements` and `advise-supervisory` sections are accurate. The `SparseArrayValue` → `Unset` comment in `value.rs` explains the write-only rationale clearly. The `write_array_elements` doc comment correctly describes the "not a preserve, a reset" semantics. The README Rust code example for `advise-supervisory` omits `use` imports for `Payload`/`MxCommandKind`/`AdviseSupervisoryCommand` but this is consistent with other README code-snippet conventions across all five clients. |
|
||||
|
||||
## Findings
|
||||
|
||||
### Client.Rust-001
|
||||
@@ -869,3 +890,53 @@ This is masked by the tests: `tls_with_require_certificate_validation_does_not_s
|
||||
**Recommendation:** Add unit tests for all three (none need a network connection).
|
||||
|
||||
**Resolution:** 2026-06-16 — Added all three test groups. (1) `--tls`/`--plaintext` resolution: `connection_defaults_to_plaintext`, `connection_tls_flag_disables_plaintext`, `connection_plaintext_flag_selects_plaintext`, `connection_rejects_tls_and_plaintext_together`. (2) Extracted the page-token dedup guard into pure `register_page_token` and covered it with `register_page_token_accepts_distinct_tokens_and_rejects_repeats`. (3) RFC3339 error paths: `rfc3339_parser_rejects_trailing_characters`, `rfc3339_parser_rejects_day_zero`, `rfc3339_parser_rejects_month_thirteen`, `rfc3339_parser_rejects_day_out_of_range_for_month`.
|
||||
|
||||
### Client.Rust-039
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| 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 |
|
||||
|
||||
**Description:** The diff adds two pieces of new public surface that are not reflected in `RustClientDesign.md`:
|
||||
|
||||
1. `Session::write_array_elements` — a new public async method in `clients/rust/src/session.rs:567-601`. It accepts `element_data_type: MxDataType`, `total_length: u32`, and `elements: impl IntoIterator<Item = (u32, MxValue)>` alongside the standard `server_handle`/`item_handle`/`user_id`. The Session API block in `RustClientDesign.md` (lines 101-131) lists every other `Session` method but omits `write_array_elements`.
|
||||
|
||||
2. `Command::AdviseSupervisory` — a new CLI subcommand (`clients/rust/crates/mxgw-cli/src/main.rs:203-214`, dispatch at lines 663-683). The CLI commands table in `RustClientDesign.md` (lines 326-353) lists every other subcommand but does not include `advise-supervisory`.
|
||||
|
||||
CLAUDE.md requires "When public APIs … change, the affected docs … must change in the same commit."
|
||||
|
||||
**Recommendation:** Add `write_array_elements` to the Session block:
|
||||
|
||||
```rust
|
||||
pub async fn write_array_elements(
|
||||
&self,
|
||||
server_handle: i32,
|
||||
item_handle: i32,
|
||||
element_data_type: MxDataType,
|
||||
total_length: u32,
|
||||
elements: impl IntoIterator<Item = (u32, MxValue)>,
|
||||
user_id: i32,
|
||||
) -> Result<(), Error>;
|
||||
```
|
||||
|
||||
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 <id> --server-handle <h> --item-handle <h>` to the CLI table.
|
||||
|
||||
### Client.Rust-040
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| 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 |
|
||||
|
||||
**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`.
|
||||
|
||||
The two unit tests that use this helper (`write_array_elements_proto_shape_has_sparse_oneof_kind` at line 1226 and `write_array_elements_empty_elements_is_valid_all_defaults` at line 1253) do not assert `data_type` on the outer `MxValue`, so they pass and do not catch the discrepancy. The only test that asserts `value.data_type == 0` is the e2e test `write_array_elements_routes_sparse_array_write_through_fake_gateway`, which correctly locks in the fix. The unit tests therefore give a false sense of coverage: they document and confirm a shape that does not match the implementation's actual output.
|
||||
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user