diff --git a/code-reviews/README.md b/code-reviews/README.md index fdaad90..14de403 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -10,23 +10,65 @@ Each module's `findings.md` is the source of truth; this file is generated from | Module | Reviewer | Date | Commit | Status | Open | Total | |---|---|---|---|---|---|---| -| [Client.Dotnet](Client.Dotnet/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 17 | -| [Client.Go](Client.Go/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 21 | -| [Client.Java](Client.Java/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 31 | -| [Client.Python](Client.Python/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 21 | -| [Client.Rust](Client.Rust/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 21 | -| [Contracts](Contracts/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 17 | -| [IntegrationTests](IntegrationTests/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 24 | -| [Server](Server/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 43 | -| [Tests](Tests/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 26 | -| [Worker](Worker/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 25 | -| [Worker.Tests](Worker.Tests/findings.md) | Claude Code | 2026-05-24 | `d692232` | Reviewed | 0 | 30 | +| [Client.Dotnet](Client.Dotnet/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 4 | 21 | +| [Client.Go](Client.Go/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 6 | 27 | +| [Client.Java](Client.Java/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 5 | 36 | +| [Client.Python](Client.Python/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 5 | 26 | +| [Client.Rust](Client.Rust/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 8 | 29 | +| [Contracts](Contracts/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 0 | 17 | +| [IntegrationTests](IntegrationTests/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 1 | 25 | +| [Server](Server/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 7 | 50 | +| [Tests](Tests/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 5 | 31 | +| [Worker](Worker/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 0 | 25 | +| [Worker.Tests](Worker.Tests/findings.md) | Claude Code | 2026-05-24 | `42b0037` | Re-reviewed | 0 | 30 | ## Pending findings Findings with status `Open` or `In Progress`, ordered by severity. -_No pending findings._ +| ID | Severity | Category | Location | Description | +|---|---|---|---|---| +| Client.Java-032 | High | Documentation & comments | `clients/java/README.md:182-183` | Commit `8738735` ("clients: document StreamAlarms + AcknowledgeAlarm in each README") added two new gradle invocations to the CLI Usage block: ``` gradle :zb-mom-ww-mxgateway-cli:run --args="stream-alarms --endpoint localhost:5000 --api-ke… | +| Client.Python-022 | High | Documentation & comments | `clients/python/README.md:201-202`, `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:389-420` | The README CLI examples added by commit `8738735` for the new alarm subcommands cite flags the CLI does not accept: ``` mxgw-py stream-alarms --session-id --max-messages 1 --json mxgw-py acknowledge-alarm --session-id --alarm-ref… | +| Client.Rust-029 | High | mxaccessgw conventions | `clients/rust/src/options.rs:98,143`; `clients/rust/src/galaxy.rs:282`; `clients/rust/src/session.rs:664-671` | `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 documen… | +| Client.Dotnet-018 | Medium | Documentation & comments | `clients/dotnet/README.md:137-138` | The README example block for the two new alarm CLI subcommands shipped in commit `11cc671` shows: ``` mxgw-dotnet stream-alarms --session-id --max-messages 1 --json mxgw-dotnet acknowledge-alarm --session-id --alarm-reference "\\… | +| Client.Go-022 | Medium | Code organization & conventions | `clients/go/cmd/mxgw-go/main.go:398-412,417-519` | Commit `8aaab82` ("Go client: port bulk read/write SDK methods + CLI subcommands") re-introduces every symptom that Client.Go-015 documented and was marked Resolved against an earlier commit: - `runWriteBulkVariant(ctx, args, stdout, stder… | +| Client.Go-023 | Medium | Concurrency & thread safety | `clients/go/cmd/mxgw-go/main.go:604-606,616-632` | `runBenchReadBulk`'s warm-up and steady-state loops are wall-clock-only again: ```go warmupDeadline := time.Now().Add(time.Duration(*warmupSeconds) * time.Second) timeout := time.Duration(*timeoutMs) * time.Millisecond for time.Now().Befor… | +| Client.Java-033 | Medium | Correctness & logic bugs | `clients/java/zb-mom-ww-mxgateway-cli/src/main/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCli.java:1078-1098` | `StreamAlarmsCommand.call()` allocates a bounded `ArrayBlockingQueue(1024)` and the gRPC observer publishes each `AlarmFeedMessage` via `queue.offer(value)`: ``` BlockingQueue queue = new ArrayBlockingQueue<>(1024); … @Over… | +| Client.Java-034 | Medium | Correctness & logic bugs | `clients/java/zb-mom-ww-mxgateway-cli/src/main/java/com/zb/mom/ww/mxgateway/cli/MxGatewayCli.java:182-198` | `BatchCommand.call()` reads one CLI invocation per stdin line and tokenises with: ``` String[] args = line.trim().split("\\s+"); … int exitCode = cmd.execute(args); ``` `split("\\s+")` does no shell-quoting parsing — it just splits on whit… | +| Client.Python-023 | Medium | Security | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:901-906` | Client.Python-013 (severity Medium, Security) was marked | +| Client.Python-024 | Medium | Code organization & conventions | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:13,48-119` | The new `batch` subcommand (commit `71d2c39`) implements the cross-language batch protocol by importing `click.testing.CliRunner` into production code and calling `runner.invoke(main, args, catch_exceptions=True)` in a `for raw_line in sys… | +| Client.Rust-022 | Medium | Correctness & logic bugs | `clients/rust/src/session.rs:369-391,403-420,427-444,452-469,476-493,631-696,706-724` | 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… | +| Client.Rust-024 | Medium | Testing coverage | `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` | 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`… | +| Server-044 | Medium | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs:216-254` | `KillWorkerAsync` is the mirror of `CloseSessionCoreAsync` for the new admin-only Kill flow, but its catch path leaks the `mxgateway.sessions.open` gauge — the exact bug that Server-006 closed for `OpenSessionAsync`. The happy path increme… | +| Tests-027 | Medium | Concurrency & thread safety | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Grpc/MxAccessGatewayServiceTests.cs:199-240`, `src/ZB.MOM.WW.MxGateway.Server/Metrics/GatewayMetrics.cs:8,73,246-251` | The review brief explicitly flagged `MxAccessGatewayServiceTests.StreamEvents_WhenEventIsWritten_RecordsSendDuration` as a known flake that "passed solo on rerun". The root cause is the `MeterListener` subscribes by `instrument.Meter.Name… | +| Client.Dotnet-019 | Low | Correctness & logic bugs | `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:745` | Client.Dotnet-005 / 010 documented (and recorded as resolved) the silent register-handle fallback pattern `reply.Register?.ServerHandle ?? reply.ReturnValue.Int32Value`, where a successful protocol+MX-status reply missing its typed `regist… | +| Client.Dotnet-020 | Low | Error handling & resilience | `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:792-810`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:774-780` | `BenchReadBulkAsync`'s steady-state `while (DateTime.UtcNow < steadyDeadline)` loop wraps each `client.InvokeAsync(...)` in a bare `catch`: ```csharp try { reply = await client.InvokeAsync( CreateCommandRequest(sessionId, readBulkMxCommand… | +| Client.Dotnet-021 | Low | Correctness & logic bugs | `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:487`, `clients/dotnet/ZB.MOM.WW.MxGateway.Client.Cli/MxGatewayClientCli.cs:715` | Both new bulk-read CLI handlers cast a signed `--timeout-ms` argument to `uint` without bounds checking: ```csharp // ReadBulkAsync (line 487) TimeoutMs = (uint)arguments.GetInt32("timeout-ms", 0), // BenchReadBulkAsync (line 715) uint tim… | +| Client.Go-024 | Low | Testing coverage | `clients/go/mxgateway/session.go:395-525`, `clients/go/mxgateway/alarms.go:65-76` | The five new bulk SDK methods on `Session` and the new `Client.StreamAlarms` method have **no unit tests** in `clients/go/mxgateway/`: - `Session.WriteBulk` (`session.go:395`) - `Session.Write2Bulk` (`session.go:418`) - `Session.WriteSecur… | +| Client.Go-025 | Low | Correctness & logic bugs | `clients/go/mxgateway/session.go:395-485,495-525` | The five new bulk methods (`WriteBulk`, `Write2Bulk`, `WriteSecuredBulk`, `WriteSecured2Bulk`, `ReadBulk`) each guard with `if entries == nil { return error }` and an upper-bound `ensureBulkSize` check, but accept a non-nil empty slice (e.… | +| Client.Go-026 | Low | Error handling & resilience | `clients/go/cmd/mxgw-go/main.go:1196-1222` | `runBatch` reads command lines with a default `bufio.Scanner`: ```go scanner := bufio.NewScanner(in) for scanner.Scan() { ... } return scanner.Err() ``` The default `bufio.Scanner` token size is 64 KiB (`bufio.MaxScanTokenSize`). One long… | +| Client.Go-027 | Low | Code organization & conventions | `clients/go/cmd/mxgw-go/main.go:1195-1206` | `runBatch`'s doc-comment says the loop "never terminates on command error; only stdin EOF (or an empty line) ends the session", and the implementation matches: ```go for scanner.Scan() { line := scanner.Text() if line == "" { break } ... }… | +| Client.Java-035 | Low | Testing coverage | `clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java` | Commit `8a0c59d` added `MxGatewayClient.streamAlarms(StreamAlarmsRequest, StreamObserver)` and a new public `MxGatewayAlarmFeedSubscription` class. No library-side test exercises either: a grep for `streamAlarms` across `… | +| Client.Java-036 | Low | Code organization & conventions | `clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayAlarmFeedSubscription.java`, `MxGatewayEventSubscription.java`, `MxGatewayActiveAlarmsSubscription.java`, `DeployEventSubscription.java` | `MxGatewayAlarmFeedSubscription` is a structural near-copy of `MxGatewayEventSubscription` — same `AtomicReference>` + `AtomicBoolean cancelled` field shape, the same `wrap(observer)` returning a `ClientResponse… | +| Client.Python-025 | Low | Testing coverage | `clients/python/tests/test_cli.py`, `clients/python/src/zb_mom_ww_mxgateway/{client.py,session.py}`, `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py` | Commits `6add4b4` and `828e3e6` added five new SDK methods (`Session.read_bulk`, `Session.write_bulk`, `Session.write2_bulk`, `Session.write_secured_bulk`, `Session.write_secured2_bulk`), `GatewayClient.stream_alarms`, the helper `_canceli… | +| Client.Python-026 | Low | Correctness & logic bugs | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:674-738` | Two minor quality issues in the new `_bench_read_bulk` body (commit `6add4b4`): 1. `import time` is done inside the function body (line 676) rather than at module top. `PythonStyleGuide.md` does not state this explicitly, but every other h… | +| Client.Rust-023 | Low | mxaccessgw conventions | `clients/rust/crates/mxgw-cli/src/main.rs:835,872,1476` | 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 87… | +| Client.Rust-025 | Low | Design-document adherence | `clients/rust/RustClientDesign.md:92-106,142-153,164-171` | 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… | +| Client.Rust-026 | Low | Performance & resource management | `clients/rust/crates/mxgw-cli/src/main.rs:1402-1406,1419-1423` | `run_bench_read_bulk` clones the `tags: Vec` 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(),… | +| Client.Rust-027 | Low | Documentation & comments | `clients/rust/.cargo/config.toml:1-9` | 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 c… | +| Client.Rust-028 | Low | mxaccessgw conventions | `clients/rust/crates/mxgw-cli/src/main.rs:1126-1166` | `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().… | +| IntegrationTests-025 | Low | Correctness & logic bugs | `src/ZB.MOM.WW.MxGateway.IntegrationTests/IntegrationTestEnvironmentTests.cs:57-84` (`ResolveRepositoryRoot_NoMarkers_ThrowsInvalidOperationExceptionNamingStartAndMarkers`) | The new regression test for IntegrationTests-022 builds an "isolated" start directory under `Path.GetTempPath()` (e.g. `C:\Users\\AppData\Local\Temp\\nested` on Windows) and calls `ResolveRepositoryRoot(isolatedStart)`, asser… | +| Server-045 | Low | Concurrency & thread safety | `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs:225,242-245`, `src/ZB.MOM.WW.MxGateway.Server/Sessions/GatewaySession.cs:837-841` | `KillWorkerAsync` reads `session.State` once into a local `bool wasClosed` (line 225) before calling `session.KillWorker(reason)`. The read is unsynchronized — `State` is a getter that takes `_syncRoot` internally so the read itself is saf… | +| Server-046 | Low | Error handling & resilience | `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs:286-307` | `ShutdownAsync` was updated to fall back to `KillWorker` when `CloseSessionCoreAsync` throws (lines 294-305) — a useful resilience improvement on its own. But the fallback's bookkeeping is wrong: `session.KillWorker(GatewayShutdownReason)`… | +| Server-047 | Low | Code organization & conventions | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/ApiKeysPage.razor:324-334`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionsPage.razor:171-195`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/Components/Pages/SessionDetailsPage.razor:231-255` | The shared `ConfirmDialog.razor` (added in `0e56b5b` / `24cc5fd`) is wired by three pages, but the pages handle `PendingAction` cleanup inconsistently: - `ApiKeysPage.ConfirmPendingAsync` captures the action, sets `PendingAction = null` sy… | +| Server-048 | Low | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs:463-498` | The two new `KillWorkerAsync_*` tests cover the happy path (`KillWorkerAsync_KillsWorkerAndRemovesSession`) and the missing-session error (`KillWorkerAsync_WhenSessionMissing_ThrowsSessionNotFound`). Three behaviorally distinct cases are m… | +| Server-049 | Low | Documentation & comments | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/IDashboardSessionAdminService.cs:5-18`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs:8-25` | `IDashboardSessionAdminService` declares three members — `CanManage`, `CloseSessionAsync`, `KillWorkerAsync` — none of which carry XML documentation. `DashboardSessionAdminService.CanManage` and the two operation methods are also undocumen… | +| Server-050 | Low | Error handling & resilience | `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs:42-75,92-125` | `CloseSessionAsync` and `KillWorkerAsync` catch only `SessionManagerException` (the `SessionNotFound` filter, then a general `SessionManagerException` catch). Anything else propagates raw to Blazor's error boundary. The propagation paths e… | +| Tests-028 | Low | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Sessions/SessionManagerTests.cs:466-496,802-807`, `src/ZB.MOM.WW.MxGateway.Server/Sessions/SessionManager.cs:216-253` | The new `KillWorkerAsync_KillsWorkerAndRemovesSession` (line 466) and `KillWorkerAsync_WhenSessionMissing_ThrowsSessionNotFound` (line 486) pin the new kill-path entry, but they do not pin the `reason` argument propagating through the chai… | +| Tests-029 | Low | Error handling & resilience | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSessionAdminServiceTests.cs:61-106,139-222`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardSessionAdminService.cs:77-125` | The new `DashboardSessionAdminServiceTests` covers the happy path and the viewer-denial path for both `CloseSessionAsync` and `KillWorkerAsync`, plus `CloseSessionAsync_WhenSessionMissing_ReportsFriendlyError` for the close-side `SessionNo… | +| Tests-030 | Low | Testing coverage | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardApiKeyManagementServiceTests.cs:115-163`, `src/ZB.MOM.WW.MxGateway.Server/Dashboard/DashboardApiKeyManagementService.cs:146-177` | The three new `DeleteAsync_*` fixtures cover unauthorised user, success path with audit, and store-refuses-with-friendly-error. They do not exercise two production behaviours: (1) `DeleteAsync_WhenStoreRefuses_ReportsFriendlyError` (line 1… | +| Tests-031 | Low | Concurrency & thread safety | `src/ZB.MOM.WW.MxGateway.Tests/Gateway/Dashboard/DashboardSnapshotPublisherTests.cs:22-61` | `ExecuteAsync_WhenSnapshotServiceThrowsOnce_ReconnectsAfterDelay` records `startedAt = DateTimeOffset.UtcNow` *before* calling `publisher.StartAsync(...)`, then asserts `secondSubscribeAt - startedAt >= reconnectDelay - 10ms` (line 59). Th… | ## Closed findings