code-reviews: re-review Client.Rust at 42b0037
Append 8 new findings (Client.Rust-022..029): MalformedReply path absent from the new bulk SDK methods, hard-coded client_correlation_id in new CLI commands, no tests for stream_alarms / bulk SDK / bench, RustClientDesign.md silent on new surface, run_bench_read_bulk clones tags inside the loop, .cargo/config.toml comment is wrong, run_batch exits on blank line, and cargo clippy fails at HEAD with three warnings the prior reviewer punted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -5,9 +5,9 @@
|
||||
| Module | `clients/rust` |
|
||||
| Reviewer | Claude Code |
|
||||
| Review date | 2026-05-24 |
|
||||
| Commit reviewed | `d692232` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 0 |
|
||||
| Commit reviewed | `42b0037` |
|
||||
| Status | Re-reviewed |
|
||||
| Open findings | 8 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -50,6 +50,52 @@ member layout unchanged. `cargo test --workspace` and `cargo clippy
|
||||
| 9 | Testing coverage | No issues found in this diff — the fake gateway's `stream_alarms` impl is a minimal stub, but no production behaviour relies on it being exercised by a test. |
|
||||
| 10 | Documentation & comments | Issues found: Client.Rust-021 (design-doc drift). |
|
||||
|
||||
### 2026-05-24 re-review (commit 42b0037)
|
||||
|
||||
Re-review pass at `42b0037`. The diff against `d692232` is dominated by
|
||||
three feature commits — `71d2c39` (ports the cross-language `batch`
|
||||
subcommand to every CLI; for Rust this adds a `Batch` clap variant plus a
|
||||
`run_batch` stdin-driven dispatch loop, and a `.cargo/config.toml` setting
|
||||
`-C link-arg=/STACK:8388608` so debug builds of the now-large clap-derive
|
||||
Command enum do not overflow Windows's default 1 MB main-thread stack);
|
||||
`3251069` (ports `BenchReadBulk` and re-adds the bulk read/write SDK
|
||||
methods to `session.rs` — `read_bulk`, `write_bulk`, `write2_bulk`,
|
||||
`write_secured_bulk`, `write_secured2_bulk` plus the matching CLI
|
||||
subcommands; this commit silently dropped the `BenchReadBulkStats`
|
||||
helper, the `failureLatencyMs`/`firstFailure` JSON enrichment, and every
|
||||
test from Client.Rust-015/016/018 that lived on the divergent branch);
|
||||
and `7de4efe` (adds `stream_alarms` / `acknowledge_alarm` raw SDK methods
|
||||
on `GatewayClient`, the `AlarmFeedStream` type alias re-exported at the
|
||||
crate root, two new CLI subcommands `stream-alarms` /
|
||||
`acknowledge-alarm`, and fixes `event_to_json` to render `MxEventFamily`
|
||||
as the protobuf enum name so the cross-language e2e parity matcher
|
||||
recognises events). `8738735` is a README-only update for the new alarm
|
||||
SDK methods + CLI examples.
|
||||
|
||||
`cargo test --workspace` is clean (26 tests across the library,
|
||||
client-behavior integration suite, and proto-fixture suite). `cargo
|
||||
clippy --workspace --all-targets -- -D warnings` **fails at HEAD** with
|
||||
three errors — the same three the previous reviewer flagged in
|
||||
Client.Rust-021's resolution as "out of scope" (see Client.Rust-029
|
||||
below): `options.rs:98,143` missing-docs, `galaxy.rs:282` clone-on-copy,
|
||||
and `session.rs:664` enum-variant-names on `BulkReplyKind`. All three
|
||||
were resolved at `a020350` (Client.Rust-001/002/012) and regressed when
|
||||
the crate rename `397d3c5` reverted parts of the prior fix; the diff
|
||||
under review does not address them.
|
||||
|
||||
| # | Category | Result |
|
||||
|---|---|---|
|
||||
| 1 | Correctness & logic bugs | Issue found: the new bulk read/write SDK methods (`read_bulk`, `write_bulk`, `write2_bulk`, `write_secured_bulk`, `write_secured2_bulk`) silently return `Vec::new()` on malformed-but-OK replies (Client.Rust-022). |
|
||||
| 2 | mxaccessgw conventions | Issues found: hard-coded correlation IDs in the new CLI subcommands `stream-alarms`, `acknowledge-alarm`, and `bench-read-bulk`'s session-close (Client.Rust-023); clippy regression at HEAD (Client.Rust-029). |
|
||||
| 3 | Concurrency & thread safety | No issues found — `stream_alarms`/`acknowledge_alarm` go through the existing `unary_request`/`stream_request` path, no new shared mutable state. The `run_batch` stdin loop uses blocking `std::io::Stdin::lock().lines()` on a tokio main task but each dispatched subcommand is spawned on a fresh task so its future does not share the main-thread stack. |
|
||||
| 4 | Error handling & resilience | No new issues beyond Client.Rust-022's malformed-OK silent default. `bench-read-bulk`'s session close is best-effort and ordered so a bench-loop error does not mask the close (`let _ = close_result;` after `bench_outcome?`). |
|
||||
| 5 | Security | No issues found — alarm `comment`/`operator` strings flow through the redaction-aware `Error::Display` path; bench's `tags` list is logged in cleartext but tags are not credentials. |
|
||||
| 6 | Performance & resource management | Issue found: `run_bench_read_bulk` clones the `Vec<String>` tag list on every warmup + steady-state iteration (Client.Rust-026). |
|
||||
| 7 | Design-document adherence | Issue found: `RustClientDesign.md` is silent on the new alarm SDK methods, the new bulk read/write SDK methods, every new CLI subcommand, and the new `.cargo/config.toml` workaround (Client.Rust-025). |
|
||||
| 8 | Code organization & conventions | No new issues — the `event_to_json` family-as-enum-name fix is a small, localised change; the bulk-write helpers are factored consistently with the existing bulk-subscribe helpers. |
|
||||
| 9 | Testing coverage | Issue found: zero tests cover `stream_alarms` on `GatewayClient`, the new bulk read/write SDK methods, or the `BenchReadBulk` flow; the fake gateway's `stream_alarms` impl drops the sender immediately (Client.Rust-024). |
|
||||
| 10 | Documentation & comments | Issue found: `.cargo/config.toml`'s comment promises "Release builds are unaffected" but the `link-arg=/STACK:8388608` setting is unconditional under `cfg(windows)` and only applies to the MSVC linker (Client.Rust-027). |
|
||||
|
||||
## Findings
|
||||
|
||||
### Client.Rust-001
|
||||
@@ -427,3 +473,201 @@ The CLI integration in Client.Rust-014 works either way; this is solely about de
|
||||
**Recommendation:** Update `RustClientDesign.md:14-33` to describe the actual flat layout: workspace root at `clients/rust/`, top-level crate `zb-mom-ww-mxgateway-client` (declared in `Cargo.toml`) with `src/lib.rs`, `src/client.rs`, etc. directly under it; `crates/mxgw-cli/` is the single member subcrate. Alternatively label the block as "Aspirational nested layout (not currently adopted)" and add a separate "Current layout" section.
|
||||
|
||||
**Resolution:** 2026-05-24 — Rewrote the "Crate Layout" section of `clients/rust/RustClientDesign.md` to describe the actual flat layout on disk instead of the aspirational nested form. The section now opens with a short paragraph stating that the workspace is rooted at `clients/rust/`, that `clients/rust/Cargo.toml` declares the top-level crate `zb-mom-ww-mxgateway-client` itself (flat layout — `src/` sits directly under the workspace root, not under `crates/`), and that the only `[workspace.members]` entry is the `mxgw-cli` binary subcrate under `crates/mxgw-cli/`. The accompanying tree replaces the fictitious `crates/zb-mom-ww-mxgateway-client/` subdirectory with the real files: `Cargo.toml`, `Cargo.lock`, `build.rs`, `README.md`, `RustClientDesign.md`, `src/{lib,client,session,galaxy,options,auth,error,value,version,generated}.rs` plus `src/generated/` (tonic-build output), `tests/{client_behavior,proto_fixtures}.rs`, and `crates/mxgw-cli/` annotated as the sole workspace member producing the `mxgw` binary. `cargo build --workspace` and `cargo test --workspace` are clean at HEAD (29 tests pass across the library, integration, and proto-fixture targets). `cargo clippy --workspace --all-targets -- -D warnings` reports three pre-existing errors at HEAD on this dev box (`options.rs:98,143` missing docs, `galaxy.rs:282` `clone_on_copy`, `session.rs:522` `enum_variant_names`) that exist independently of this doc-only change — verified by running clippy with the design-doc edit stashed; tracking those is out of scope for Client.Rust-021.
|
||||
|
||||
### Client.Rust-022
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| 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 |
|
||||
|
||||
**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:
|
||||
|
||||
```rust
|
||||
Ok(match reply.payload {
|
||||
Some(mx_command_reply::Payload::ReadBulk(reply)) => reply.results,
|
||||
_ => Vec::new(),
|
||||
})
|
||||
```
|
||||
|
||||
and `bulk_write_results` does the same for the four write families. A caller of `write_bulk` / `read_bulk` on a malformed-but-OK reply therefore sees an empty result vector and cannot distinguish "zero items processed" from "gateway returned a shapeless reply". This is exactly the anti-pattern Client.Rust-006 fixed in `0d8a28d` (which added an `Error::MalformedReply` variant and made `bulk_results` return `Result<Vec<…>, Error>`), and which Client.Rust-005 fixed for `register_server_handle` / `add_item_handle` / `add_item2_handle` (those three also now fall back to `unwrap_or_default()` returning handle `0` at `session.rs:631-660`, regressing alongside the new methods). The fix lived on the divergent branch and was lost when the d692232 line of history was re-platformed without picking up the post-`a020350` Resolve commits — `Error::MalformedReply` is absent from `clients/rust/src/error.rs` at HEAD.
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-023
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | mxaccessgw conventions |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:835,872,1476` |
|
||||
| Status | Open |
|
||||
|
||||
**Description:** Three CLI subcommands added since `d692232` hard-code their `client_correlation_id`:
|
||||
|
||||
```rust
|
||||
client_correlation_id: "rust-cli-stream-alarms".to_owned(), // line 835
|
||||
client_correlation_id: "rust-cli-acknowledge-alarm".to_owned(), // line 872
|
||||
client_correlation_id: "rust-cli-bench-read-bulk-close".to_owned(), // line 1476
|
||||
```
|
||||
|
||||
Every invocation of these subcommands on the same machine — and every iteration of the cross-language e2e matrix that exercises them concurrently — produces requests with identical correlation IDs in gateway logs, defeating the diagnostic value Client.Rust-011/014 unlocked when `next_correlation_id` was made `pub`. (As noted in the d692232 review-row header, the prior `rust-cli-ping`/`rust-cli-close-session` hard-codes are also still in place at lines 506 and 553 after the rename commit reverted the Client.Rust-014 fix; the three new ones extend the same regression to new CLI surface.)
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-024
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| 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 |
|
||||
|
||||
**Description:** The diff under review adds substantial SDK and CLI surface with no positive-path coverage:
|
||||
|
||||
1. **`GatewayClient::stream_alarms`** (client.rs:280-291) has no test. The fake gateway's `stream_alarms` impl in `tests/client_behavior.rs:408-415` immediately drops the unique sender and returns an empty `ReceiverStream`, so an attached client would observe a clean end-of-stream — nothing exercises the snapshot → `snapshot_complete` → `transition` sequence that the doc on `AlarmFeedStream` advertises, mid-stream `Status` mapping through `Error::from`, or the cooperative-cancel-on-drop contract.
|
||||
2. **Five new bulk SDK methods** (`read_bulk`, `write_bulk`, `write2_bulk`, `write_secured_bulk`, `write_secured2_bulk` on `Session`) have **zero** tests. The fake gateway's `Invoke` dispatcher does not handle any of `MxCommandKind::ReadBulk`, `WriteBulk`, `Write2Bulk`, `WriteSecuredBulk`, `WriteSecured2Bulk`, so a happy-path round trip would fail at the gateway-side match. The malformed-reply branches in Client.Rust-022 are therefore also untested.
|
||||
3. **`run_bench_read_bulk`** has no test. The `BenchReadBulkStats` helper that Client.Rust-015/016 added at `a020350` (with its own dedicated unit test) was dropped in the port and there is no replacement — the percentile / mean math, the success-vs-failure accounting, and the JSON schema that `scripts/bench-read-bulk.ps1` collates are all untested.
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-025
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `clients/rust/RustClientDesign.md:92-106,142-153,164-171` |
|
||||
| Status | Open |
|
||||
|
||||
**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`:
|
||||
|
||||
- **SDK methods** on `GatewayClient`: `stream_alarms` and `acknowledge_alarm`, plus the new `AlarmFeedStream` type alias re-exported at the crate root in `lib.rs:27`.
|
||||
- **SDK methods** on `Session`: `read_bulk`, `write_bulk`, `write2_bulk`, `write_secured_bulk`, `write_secured2_bulk`. The `Session` impl block in `RustClientDesign.md:92-106` still lists only the original six bulk-subscribe helpers, identical to its d692232 state (the Client.Rust-019 resolution that added these to the doc lived on the divergent branch and was lost in the `397d3c5` rename).
|
||||
- **CLI subcommands**: `stream-alarms`, `acknowledge-alarm`, `read-bulk`, `write-bulk`, `write2-bulk`, `write-secured-bulk`, `write-secured2-bulk`, `bench-read-bulk`, and `batch`. The `Commands` block in `RustClientDesign.md:166-171` still lists only `version`, `smoke`, `stream-events`, `write`.
|
||||
- **Error variants**: `Error::MalformedReply` / `Error::Unavailable` / `Error::InvalidEndpoint` are still missing from the `Error` enum sketch at `RustClientDesign.md:142-153` (Client.Rust-017's expansion was also lost in the rename — and Client.Rust-022 will add `MalformedReply` back when fixed).
|
||||
- **Build configuration**: the new `clients/rust/.cargo/config.toml` Windows 8 MB stack workaround is not mentioned anywhere in the design doc — a reader reproducing the build on a fresh dev box would not know to expect the stack-overflow failure mode the file documents.
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-026
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Performance & resource management |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:1402-1406,1419-1423` |
|
||||
| Status | Open |
|
||||
|
||||
**Description:** `run_bench_read_bulk` clones the `tags: Vec<String>` on every iteration of both the warmup loop and the steady-state measurement loop:
|
||||
|
||||
```rust
|
||||
while Instant::now() < warmup_deadline {
|
||||
let _ = session
|
||||
.read_bulk(server_handle, tags.clone(), timeout_ms_param)
|
||||
.await;
|
||||
}
|
||||
// ...
|
||||
while Instant::now() < steady_deadline {
|
||||
let call_start = Instant::now();
|
||||
let result = session
|
||||
.read_bulk(server_handle, tags.clone(), timeout_ms_param)
|
||||
.await;
|
||||
let elapsed = call_start.elapsed();
|
||||
latencies_ms.push(elapsed.as_secs_f64() * 1000.0);
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
Each clone allocates one fresh `Vec<String>` plus `bulk_size` heap-allocated `String` clones (default 6, but the flag goes higher; the cross-language matrix routinely runs with `bulk_size=64` and `bulk_size=256`). The allocation happens **inside** `call_start ... call_start.elapsed()`, so the per-call latency the bench reports includes the clone cost — both inflating the absolute number and adding allocator-noise variance to the percentile distribution that the cross-language driver compares against the .NET/Go/Java/Python bench numbers (none of which allocate the tag list per call).
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-027
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `clients/rust/.cargo/config.toml:1-9` |
|
||||
| Status | Open |
|
||||
|
||||
**Description:** The new build-config file added by `71d2c39` carries this leading comment:
|
||||
|
||||
```
|
||||
[target.'cfg(windows)']
|
||||
# Bump the default 1 MB Windows stack to 8 MB. clap-derive builds a large
|
||||
# Command enum in this CLI (one variant per subcommand, each carrying flag
|
||||
# args); in debug builds the enum is materialized on the stack without
|
||||
# optimization and overflows the default Windows main-thread stack before
|
||||
# even reaching our code. Release builds are unaffected but the e2e matrix
|
||||
# drives the CLI through `cargo run` (debug), so the link-arg ships with
|
||||
# every dev-time invocation.
|
||||
rustflags = ["-C", "link-arg=/STACK:8388608"]
|
||||
```
|
||||
|
||||
Two issues with the documentation vs the configuration:
|
||||
|
||||
1. The comment claims "Release builds are unaffected" but the `rustflags` setting is unconditional under `cfg(windows)` — `cargo build --release` on Windows also produces a binary whose `IMAGE_OPTIONAL_HEADER.SizeOfStackReserve` is 8 MB. The 8 MB stack reservation is in fact harmless for release builds (the optimizer removes the enum from the stack), but the comment misleads a reader trying to understand whether release artifacts of `mxgw` ship with a non-default header value (they do).
|
||||
2. `/STACK:` is an MSVC-linker (`link.exe` / `lld-link`) directive; `cargo build` on Windows with `+gnu` targets (mingw) routes link args through the GNU linker which rejects `/STACK:` and instead expects `-Wl,--stack,8388608`. The current file silently breaks `x86_64-pc-windows-gnu` builds — a configuration `docs/ToolchainLinks.md` lists no expectation about, but a reader picking up the file does not see the constraint.
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-028
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | mxaccessgw conventions |
|
||||
| Location | `clients/rust/crates/mxgw-cli/src/main.rs:1126-1166` |
|
||||
| Status | Open |
|
||||
|
||||
**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:
|
||||
|
||||
1. **Empty-line sentinel**: `if line.is_empty() { break; }` exits the loop on the first empty line, terminating the batch session. The comment block above declares this as a feature ("stdin EOF (or an empty line) ends the session") but an accidental blank line from any driver — for example a `Write-Output ""` between commands in the PowerShell e2e matrix — silently ends the session and the harness has no way to distinguish "end-of-input" from "the CLI broke after the previous command". The other four language CLIs that implement `batch` use only EOF, not an empty-line sentinel.
|
||||
2. **Blocking on a tokio worker**: any non-trivial latency in `dispatch` (e.g. a 30-second `bench-read-bulk`) keeps `run_batch` parked on its `JoinHandle.await`, which is fine; but if `dispatch` ever needs to drive the stdin reader (it doesn't today, but the `Batch` subcommand is the natural seam for future "stream input to a subcommand"), the blocking `Stdin::lock()` on the main task would deadlock the runtime worker.
|
||||
|
||||
**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.
|
||||
|
||||
### Client.Rust-029
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| 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 |
|
||||
|
||||
**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:
|
||||
|
||||
```
|
||||
error: missing documentation for a method
|
||||
--> src\options.rs:98:5
|
||||
|
|
||||
98 | pub fn with_max_grpc_message_bytes(mut self, max_grpc_message_bytes: usize) -> Self {
|
||||
|
||||
error: missing documentation for a method
|
||||
--> src\options.rs:143:5
|
||||
|
|
||||
143 | pub fn max_grpc_message_bytes(&self) -> usize {
|
||||
|
||||
error: using `clone` on type `Option<Timestamp>` which implements the `Copy` trait
|
||||
--> src\galaxy.rs:282:24
|
||||
|
||||
error: all variants have the same postfix: `Bulk`
|
||||
--> src\session.rs:664:1
|
||||
|
|
||||
664 | / enum BulkReplyKind {
|
||||
665 | | AddItemBulk,
|
||||
666 | | AdviseItemBulk,
|
||||
667 | | RemoveItemBulk,
|
||||
```
|
||||
|
||||
All three were resolved at `a020350` (Client.Rust-001, Client.Rust-002, Client.Rust-012 respectively) but regressed when `397d3c5` re-platformed parts of the prior fix and were never re-fixed. CLAUDE.md explicitly mandates that `cargo clippy --workspace --all-targets -- -D warnings` pass cleanly; the documented build/test workflow for the module breaks. The prior reviewer correctly noted the regressions but routed them as "out of scope for Client.Rust-021", which left them invisible to the open-findings count. They are tracked here so they appear in the pending-findings index and are not lost again.
|
||||
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user