diff --git a/code-reviews/Driver.Modbus.Cli/findings.md b/code-reviews/Driver.Modbus.Cli/findings.md index 822595d1..86ff0af3 100644 --- a/code-reviews/Driver.Modbus.Cli/findings.md +++ b/code-reviews/Driver.Modbus.Cli/findings.md @@ -4,12 +4,12 @@ |---|---| | Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | -## Checklist coverage +## Checklist coverage (initial review 2026-05-22, commit `76d35d1`) A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank. @@ -27,6 +27,25 @@ a category produced nothing rather than leaving it blank. | 9 | Testing coverage | Driver.Modbus.Cli-008 | | 10 | Documentation & comments | No issues found | +## Re-review 2026-06-19 (commit 7286d320) + +All eight prior findings were Resolved. This re-review covers the full module at HEAD. + +#### Checklist coverage (re-review 2026-06-19) + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.Modbus.Cli-011 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | Driver.Modbus.Cli-009 | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No new gaps (72 tests passing) | +| 10 | Documentation & comments | Driver.Modbus.Cli-010 (input validation gap, low) | + ## Findings ### Driver.Modbus.Cli-001 @@ -278,3 +297,84 @@ previously untested branch logic: `ReadCommand` / `WriteCommand` for finding 005. Total test count grew from 18 to 64; all pass. + +### Driver.Modbus.Cli-009 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:68`; `ReadCommand.cs:86`; `WriteCommand.cs:112`; `SubscribeCommand.cs:129` | +| Status | Resolved | + +**Description:** `FlushLogging()` (defined in `DriverCommandBase`) is never called in any of +the four Modbus CLI command `finally` blocks. Serilog's default console sink buffers output; +without `Log.CloseAndFlush()` before process exit, log lines emitted during +`ShutdownAsync` (especially on Ctrl+C of a long-running `subscribe`) can be silently +dropped. The sibling `Driver.AbCip.Cli` calls `FlushLogging()` in every command's `finally` +block (with a `// Driver.AbCip.Cli-005` reference to the finding that introduced it); the +Modbus CLI was never brought in line with that pattern. + +**Recommendation:** Add `FlushLogging();` as the last statement in the `finally` block of +each command's `ExecuteAsync`, mirroring `Driver.AbCip.Cli`. No test is needed for this +mechanical addition — the behaviour (Serilog flush) is not unit-testable without mocking +the global logger. + +**Resolution:** Resolved 2026-06-19 — added `FlushLogging();` as the last statement in the +`finally` block of `ProbeCommand`, `ReadCommand`, `WriteCommand`, and `SubscribeCommand`, +each annotated with a `// Driver.Modbus.Cli-009` comment. Mirrors `Driver.AbCip.Cli`. + +### Driver.Modbus.Cli-010 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:36` | +| Status | Resolved | + +**Description:** `SubscribeCommand`'s `--interval-ms` option (`int IntervalMs`, default 1000) +has no validation guard. Passing `--interval-ms 0` or a negative value produces a +non-positive `TimeSpan.FromMilliseconds(IntervalMs)` handed to `driver.SubscribeAsync`, which +propagates deep into `PollGroupEngine` and surfaces as an opaque downstream failure rather +than a clean operator error. The sibling `Driver.AbCip.Cli.SubscribeCommand` introduced +`ValidateInterval(int intervalMs)` (Driver.AbCip.Cli-004) to guard this; the Modbus CLI +was never updated to match. + +**Recommendation:** Add an `internal static void ValidateInterval(int intervalMs)` method to +`SubscribeCommand` (mirroring the AbCip sibling) that throws `CommandException` for +non-positive values, and call it at the top of `ExecuteAsync` after `ValidateEndpoint()`. + +**Resolution:** Resolved 2026-06-19 — added `SubscribeCommand.ValidateInterval(int intervalMs)` +(throws `CommandException` for `<= 0`) and called it after `ValidateEndpoint()` in +`ExecuteAsync`. New `SubscribeCommandValidationTests` class with 3+4+1 = 8 tests covers the +valid / invalid boundary and the end-to-end rejection via `ExecuteAsync`. Total test count +grew from 64 to 72. + +### Driver.Modbus.Cli-011 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:119-122` | +| Status | Resolved | + +**Description:** The "Subscribed to … Ctrl+C to stop." banner was printed (`WriteLineAsync`) +**after** `driver.SubscribeAsync` returned, meaning the `PollGroupEngine` background thread +could already be delivering `OnDataChange` events to `console.Output.WriteLine` while the +banner was still being awaited on the main thread. `TextWriter.WriteLine` is not guaranteed +thread-safe, so the banner and the first change-event line could interleave or tear. The +sibling `Driver.AbCip.Cli.SubscribeCommand` explicitly prints the banner **before** wiring +`OnDataChange` (with a `// Driver.AbCip.Cli-003` comment explaining the ordering contract); +the Modbus CLI was never updated to match. + +**Recommendation:** Move the `WriteLineAsync` banner to immediately after `InitializeAsync` +and before `OnDataChange` is wired, so the main-thread write is guaranteed to complete before +the poll thread can write change events. This matches the AbCip ordering. + +**Resolution:** Resolved 2026-06-19 — moved the banner `WriteLineAsync` to immediately after +`InitializeAsync` and before the `OnDataChange` handler is attached, eliminating the race +with the first poll-thread event. Annotated with `// Driver.Modbus.Cli-011` reference. +No separate test required — the fix is a purely structural ordering change, and the existing +`CommandCancellationTests` continue to pass. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs index cc64461e..a2b83cac 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs @@ -68,6 +68,9 @@ public sealed class ProbeCommand : ModbusCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + // Driver.Modbus.Cli-009: flush Serilog before process exit so buffered log lines + // emitted during driver shutdown are not lost. Matches Driver.AbCip.Cli pattern. + FlushLogging(); } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs index eed11fd7..bed668db 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs @@ -86,6 +86,9 @@ public sealed class ReadCommand : ModbusCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + // Driver.Modbus.Cli-009: flush Serilog before process exit so buffered log lines + // emitted during driver shutdown are not lost. Matches Driver.AbCip.Cli pattern. + FlushLogging(); } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs index f793a8b8..ee96c573 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs @@ -63,6 +63,9 @@ public sealed class SubscribeCommand : ModbusCommandBase { ConfigureLogging(); ValidateEndpoint(); + // Driver.Modbus.Cli-010: reject non-positive interval before opening the driver so + // the operator gets a clean error instead of a confusing PollGroupEngine failure. + ValidateInterval(IntervalMs); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(Region, Address, DataType); @@ -87,6 +90,14 @@ public sealed class SubscribeCommand : ModbusCommandBase { await driver.InitializeAsync("{}", ct); + // Driver.Modbus.Cli-011: emit the banner BEFORE wiring OnDataChange so the + // main-thread WriteLineAsync cannot interleave with poll-thread change-event + // writes. TextWriter.WriteLine is not guaranteed thread-safe; once the handler + // is attached and SubscribeAsync starts, change events run on the poll thread. + // Mirrors the ordering in Driver.AbCip.Cli.SubscribeCommand. + await console.Output.WriteLineAsync( + $"Subscribed to {tagName} @ {IntervalMs}ms. Ctrl+C to stop."); + // Route every data-change event to the CliFx console (not System.Console — the // analyzer flags it + IConsole is the testable abstraction). driver.OnDataChange += (_, e) => @@ -114,9 +125,6 @@ public sealed class SubscribeCommand : ModbusCommandBase }; handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct); - - await console.Output.WriteLineAsync( - $"Subscribed to {tagName} @ {IntervalMs}ms. Ctrl+C to stop."); try { await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct); @@ -134,6 +142,26 @@ public sealed class SubscribeCommand : ModbusCommandBase catch { /* teardown best-effort */ } } await driver.ShutdownAsync(CancellationToken.None); + // Driver.Modbus.Cli-009: flush Serilog before process exit so buffered log lines + // emitted just before Ctrl+C (e.g. during UnsubscribeAsync / ShutdownAsync) are + // not lost. Matches the pattern in Driver.AbCip.Cli commands. + FlushLogging(); } } + + /// + /// Driver.Modbus.Cli-010: guards --interval-ms against zero or negative values. + /// A non-positive interval would produce a non-positive into + /// SubscribeAsync; the CLI should fail fast with an actionable error rather + /// than relying on the downstream PollGroupEngine to clamp the value. + /// Mirrors Driver.AbCip.Cli.SubscribeCommand.ValidateInterval. + /// + /// The polling interval in milliseconds. + internal static void ValidateInterval(int intervalMs) + { + if (intervalMs <= 0) + throw new CliFx.Exceptions.CommandException( + $"--interval-ms must be > 0 (got {intervalMs}). " + + "PollGroupEngine floors sub-250ms values, but accepts any positive integer."); + } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs index e1b5b153..7db233b8 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs @@ -112,6 +112,9 @@ public sealed class WriteCommand : ModbusCommandBase finally { await driver.ShutdownAsync(CancellationToken.None); + // Driver.Modbus.Cli-009: flush Serilog before process exit so buffered log lines + // emitted during driver shutdown are not lost. Matches Driver.AbCip.Cli pattern. + FlushLogging(); } } diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/SubscribeCommandValidationTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/SubscribeCommandValidationTests.cs new file mode 100644 index 00000000..b03f1127 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/SubscribeCommandValidationTests.cs @@ -0,0 +1,62 @@ +using CliFx.Infrastructure; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests; + +/// +/// Covers Driver.Modbus.Cli-010: must reject a non-positive +/// --interval-ms before touching the driver so the operator gets a clear +/// rather than a downstream failure deep +/// inside PollGroupEngine. Mirrors the ValidateInterval guard in +/// Driver.AbCip.Cli.SubscribeCommand. +/// +[Trait("Category", "Unit")] +public sealed class SubscribeCommandValidationTests +{ + /// Verifies that ValidateInterval rejects zero and negative interval values. + /// The invalid interval in milliseconds to test. + [Theory] + [InlineData(0)] + [InlineData(-1)] + [InlineData(-1000)] + public void ValidateInterval_rejects_non_positive_interval(int intervalMs) + { + Should.Throw( + () => SubscribeCommand.ValidateInterval(intervalMs)); + } + + /// Verifies that ValidateInterval accepts strictly positive interval values. + /// The valid interval in milliseconds to test. + [Theory] + [InlineData(1)] + [InlineData(250)] + [InlineData(1000)] + [InlineData(5000)] + public void ValidateInterval_accepts_positive_interval(int intervalMs) + { + Should.NotThrow(() => SubscribeCommand.ValidateInterval(intervalMs)); + } + + /// + /// Verifies that ExecuteAsync throws + /// when --interval-ms is non-positive, before any driver I/O. + /// + [Fact] + public async Task ExecuteAsync_rejects_zero_interval_before_driver_connect() + { + var sut = new SubscribeCommand + { + Host = "127.0.0.1", + Region = ModbusRegion.HoldingRegisters, + Address = 0, + DataType = ModbusDataType.UInt16, + IntervalMs = 0, + }; + using var console = new FakeInMemoryConsole(); + + await Should.ThrowAsync( + async () => await sut.ExecuteAsync(console)); + } +}