review(Driver.Modbus.Cli): FlushLogging() + interval validation + banner-before-events

Re-review at 7286d320. -009 FlushLogging() in finally; -010 validate --interval-ms positive (+8 tests); -011 print subscribe banner before wiring OnDataChange (no main/poll-thread interleave). Parity with AbCip.Cli.
2026-06-19 12:08:45 -04:00
parent 754c5a3684
commit b0f9b8016a
6 changed files with 205 additions and 6 deletions
@@ -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.
@@ -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();
        }
    }

@@ -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();
        }
    }

@@ -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();
        }
    }
+
+    /// <summary>
+    ///     Driver.Modbus.Cli-010: guards <c>--interval-ms</c> against zero or negative values.
+    ///     A non-positive interval would produce a non-positive <see cref="TimeSpan"/> into
+    ///     <c>SubscribeAsync</c>; the CLI should fail fast with an actionable error rather
+    ///     than relying on the downstream <c>PollGroupEngine</c> to clamp the value.
+    ///     Mirrors <c>Driver.AbCip.Cli.SubscribeCommand.ValidateInterval</c>.
+    /// </summary>
+    /// <param name="intervalMs">The polling interval in milliseconds.</param>
+    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.");
+    }
 }
@@ -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();
        }
    }

@@ -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;
+
+/// <summary>
+///     Covers Driver.Modbus.Cli-010: <see cref="SubscribeCommand"/> must reject a non-positive
+///     <c>--interval-ms</c> before touching the driver so the operator gets a clear
+///     <see cref="CliFx.Exceptions.CommandException"/> rather than a downstream failure deep
+///     inside <c>PollGroupEngine</c>. Mirrors the <c>ValidateInterval</c> guard in
+///     <c>Driver.AbCip.Cli.SubscribeCommand</c>.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class SubscribeCommandValidationTests
+{
+    /// <summary>Verifies that ValidateInterval rejects zero and negative interval values.</summary>
+    /// <param name="intervalMs">The invalid interval in milliseconds to test.</param>
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    [InlineData(-1000)]
+    public void ValidateInterval_rejects_non_positive_interval(int intervalMs)
+    {
+        Should.Throw<CliFx.Exceptions.CommandException>(
+            () => SubscribeCommand.ValidateInterval(intervalMs));
+    }
+
+    /// <summary>Verifies that ValidateInterval accepts strictly positive interval values.</summary>
+    /// <param name="intervalMs">The valid interval in milliseconds to test.</param>
+    [Theory]
+    [InlineData(1)]
+    [InlineData(250)]
+    [InlineData(1000)]
+    [InlineData(5000)]
+    public void ValidateInterval_accepts_positive_interval(int intervalMs)
+    {
+        Should.NotThrow(() => SubscribeCommand.ValidateInterval(intervalMs));
+    }
+
+    /// <summary>
+    ///     Verifies that ExecuteAsync throws <see cref="CliFx.Exceptions.CommandException"/>
+    ///     when <c>--interval-ms</c> is non-positive, before any driver I/O.
+    /// </summary>
+    [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<CliFx.Exceptions.CommandException>(
+            async () => await sut.ExecuteAsync(console));
+    }
+}