Joseph Doherty
b0f9b8016a
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.
19 KiB
Code Review — Driver.Modbus.Cli
| Field | Value |
|---|---|
| Module | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | 7286d320 |
| Status | Reviewed |
| Open findings | 0 |
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.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.Modbus.Cli-001, Driver.Modbus.Cli-002, Driver.Modbus.Cli-003 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.Modbus.Cli-004 |
| 4 | Error handling & resilience | Driver.Modbus.Cli-005, Driver.Modbus.Cli-006 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | Driver.Modbus.Cli-007 |
| 8 | Code organization & conventions | No issues found |
| 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
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:43-51 |
| Status | Resolved |
Description: SubscribeCommand synthesises its ModbusTagDefinition with only
Name, Region, Address, DataType, Writable, and ByteOrder — it never
exposes or passes --bit-index, --string-length, or --string-byte-order.
A user running subscribe -t BitInRegister always watches bit 0 regardless of
intent, and subscribe -t String runs with StringLength = 0. The doc
(docs/Driver.Modbus.Cli.md) lists BitInRegister, String, Bcd16, Bcd32
in the subscribe --type help text, so these types are advertised as supported
but cannot be used correctly. read and write both expose all three flags;
subscribe is the odd one out.
Recommendation: Add --bit-index, --string-length, and --string-byte-order
options to SubscribeCommand (mirroring ReadCommand) and pass them into the
ModbusTagDefinition, or trim the --type help text to the types subscribe
actually supports and reject BitInRegister / String at command entry with a
clear message.
Resolution: Resolved 2026-05-22 — added --bit-index, --string-length, and --string-byte-order options to SubscribeCommand, mirroring ReadCommand, and passed them through to ModbusTagDefinition so BitInRegister and String types subscribe correctly.
Driver.Modbus.Cli-002
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/WriteCommand.cs:54-89 |
| Status | Resolved |
Description: WriteCommand rejects read-only regions (DiscreteInputs /
InputRegisters) but does not validate that --type is meaningful for the
Coils region. write -r Coils -a 5 -t UInt16 -v 42 builds a Coils tag with
DataType = UInt16; the value parses to a boxed ushort, and the driver's
WriteOneAsync coil branch calls Convert.ToBoolean(value) which succeeds for
any non-zero ushort (yields true). The write silently lands as a coil ON with
no diagnostic, even though the operator asked for a 16-bit register write. A coil
region only supports Bool-style boolean values.
Recommendation: After the read-only-region check, reject Region == Coils
combined with any non-boolean --type (anything other than Bool), with a
message explaining coils carry a single bit.
Resolution: Resolved 2026-05-22 — added a Region == Coils && DataType != Bool check immediately after the read-only-region guard, throwing CommandException with a message explaining that coils carry a single bit and only --type Bool is valid.
Driver.Modbus.Cli-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs:14-24 |
| Status | Resolved |
Description: Port (int) and TimeoutMs (int) accept any 32-bit value,
including negatives and ports above 65535. UnitId is a byte, so it accepts
0-255 even though the option description and docs/Driver.Modbus.Cli.md both say
the valid range is 1-247 (0 is the Modbus broadcast address; 248-255 are
reserved). A negative --timeout-ms becomes a negative TimeSpan passed straight
into the driver; an out-of-range --port fails later with an opaque socket
error. None of these are validated at parse time.
Recommendation: Validate Port (1-65535), TimeoutMs (greater than 0), and
UnitId (1-247) at the top of each command's ExecuteAsync (or in
ModbusCommandBase), throwing CliFx.Exceptions.CommandException with a clear
message — consistent with how WriteCommand already rejects bad regions and
boolean strings.
Resolution: Resolved 2026-05-23 — added ModbusCommandBase.ValidateEndpoint()
that throws CliFx.Exceptions.CommandException for --port outside 1..65535,
non-positive --timeout-ms, and --unit-id outside 1..247 (Modbus spec unicast
range — rejects the broadcast 0 and reserved 248-255). Each of ProbeCommand,
ReadCommand, WriteCommand, and SubscribeCommand now calls it at the top of
ExecuteAsync after ConfigureLogging(). Regression tests live in
ModbusCommandBaseTests (range + boundary cases for all three knobs).
Driver.Modbus.Cli-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:61-67 |
| Status | Resolved |
Description: The OnDataChange handler is invoked from the driver's
PollGroupEngine background thread and calls console.Output.WriteLine
synchronously. An exception thrown inside this handler (e.g. an IOException on a
redirected or closed stdout) propagates on the poll-engine thread and is not
caught — it could fault the background loop. For a long-running subscribe this
is a real, if low-probability, crash path. Output lines are also written without
any synchronization, so overlapping poll ticks could interleave partial lines.
Recommendation: Wrap the handler body in a try/catch that swallows or logs
write failures so a transient console-write error cannot tear down the poll loop.
A single lock around the write also removes the interleave risk.
Resolution: Resolved 2026-05-23 — wrapped the OnDataChange handler in
try/catch (Exception) that logs to Serilog at Warning and swallows the failure
so a transient console-write error cannot fault the PollGroupEngine background
loop. The console write is also serialized through a local lock object, removing
the partial-line interleave risk when multiple poll ticks overlap.
Driver.Modbus.Cli-005
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:21-54; Commands/ReadCommand.cs:46-75; Commands/WriteCommand.cs:54-89 |
| Status | Resolved |
Description: All three commands call ConfigureLogging() then
console.RegisterCancellationHandler(), but if the operator presses Ctrl+C
before InitializeAsync completes, the resulting OperationCancelledException
propagates out of ExecuteAsync unhandled. CliFx renders unhandled non-
CommandException exceptions as a full stack trace, which is noisy for what is
just a user-cancelled run. SubscribeCommand correctly catches
OperationCancelledException around its Task.Delay, but the connect/read/write
commands do not catch it around their driver calls.
Recommendation: Either let cancellation surface a clean message (catch
OperationCancelledException in each command and exit quietly) or document that
the noisy trace on Ctrl+C-during-connect is acceptable. Consistency with
SubscribeCommand's handling is the cleaner choice.
Resolution: Resolved 2026-05-23 — added a
catch (OperationCanceledException) when (ct.IsCancellationRequested) clause
around the driver-call bodies in ProbeCommand, ReadCommand, and WriteCommand
that prints Cancelled. and falls through to the existing finally-block
shutdown. Matches SubscribeCommand's existing handling so all four commands now
exit quietly on Ctrl+C. Regression tests in CommandCancellationTests pre-cancel
the CliFx FakeInMemoryConsole before calling ExecuteAsync and assert no
exception escapes.
Driver.Modbus.Cli-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:35-53 |
| Status | Resolved |
Description: probe reports Health: {health.State} from GetHealth().
After a successful InitializeAsync the driver sets state to Healthy
regardless of whether the subsequent probe register read returns Good or a Bad
status code. ReadAsync does not throw on a Modbus exception response — it
returns a DataValueSnapshot with a Bad StatusCode. So probe against a host
that accepts the TCP connection but rejects FC03 at the probe address prints
Health: Healthy while the snapshot line below shows a Bad status. The two lines
disagree, and the headline Health value (the thing an operator scans first)
overstates success. The doc bills probe as the "is the PLC up + talking Modbus"
check, which the bare Healthy does not actually confirm.
Recommendation: Have probe derive its headline verdict from the probe
snapshot's StatusCode (Good vs Bad) rather than — or in addition to — the driver
State, or print a single combined verdict line so the two cannot contradict each
other.
Resolution: Resolved 2026-05-23 — ProbeCommand now prints a single combined
Verdict: line above the bare driver-state line; the headline is computed by a
new ProbeCommand.ComputeVerdict(DriverState, statusCode) helper that combines
the driver state with the probe snapshot's OPC UA quality class (top 2 bits of
the StatusCode). Verdict is FAIL whenever the driver is not Healthy OR the
snapshot is Bad, DEGRADED when the driver is Healthy but the snapshot is
Uncertain, and OK only when both are Good. Regression tests in
ProbeCommandTests pin the verdict-grid behaviour.
Driver.Modbus.Cli-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | docs/Driver.Modbus.Cli.md:124-156; src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs |
| Status | Resolved |
Description: docs/Driver.Modbus.Cli.md devotes a whole "v2 addressing
grammar" section to the industry-standard tag-address strings (40001:F:CDAB,
HR1:I, C100, V2000:F:CDAB, etc.) and says "set the per-tag addressString
field instead of the structured region + address + dataType fields." None of
the CLI commands expose an --address-string (or equivalent) flag — read,
write, and subscribe only accept the structured --region + --address +
--type triple. The documented address-string grammar is reachable only through a
hand-written DriverConfig JSON, not through this CLI. The doc reads as if the CLI
supports it.
Recommendation: Either add an --address-string option that feeds the
driver's address-string parser (and --family for the DL205/MELSEC native
syntax), or scope the "v2 addressing grammar" section of the doc to note it
applies to DriverConfig JSON and is not a CLI flag.
Resolution: Resolved 2026-05-23 — chose the doc-scoping fix (an
--address-string flag is a bigger feature that warrants its own design
discussion). Added a "CLI scope" callout to the top of the
docs/Driver.Modbus.Cli.md "v2 addressing grammar" section stating the CLI
accepts only the structured --region + --address + --type triple and that
the address-string grammar is a DriverConfig JSON feature. The pre-existing
closing paragraph already said the same thing; the new callout makes it visible
before the grammar examples instead of after them. Code surface left unchanged.
Driver.Modbus.Cli-008
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/ |
| Status | Resolved |
Description: The test project covers only the two pure-function seams:
ReadCommand.SynthesiseTagName and WriteCommand.ParseValue. There is no coverage
for WriteCommand's read-only-region rejection (Region is not (Coils or HoldingRegisters)), no test for ModbusCommandBase.BuildOptions (e.g. that
Probe.Enabled is false and AutoReconnect tracks --disable-reconnect), and
no test asserting unsupported write types throw. The branch logic in
WriteCommand.ExecuteAsync and ModbusCommandBase.BuildOptions is the part most
likely to regress and is currently untested. The validation gaps in findings
002/003 are also untested precisely because no test exercises that path.
Recommendation: Add tests for WriteCommand's region-validation branch and for
ModbusCommandBase.BuildOptions (construct a command instance via the init
setters and assert the produced ModbusDriverOptions). Once findings 002/003 are
fixed, add tests for the new validation paths.
Resolution: Resolved 2026-05-23 — added four new test classes covering the previously untested branch logic:
ModbusCommandBaseTests— six tests overBuildOptions(probe disabled,TimeoutMs→Timeoutmapping,AutoReconnecttracking--disable-reconnectin both directions, host/port/unit flow-through, tag pass-through) plus the newValidateEndpointrange-check tests for finding 003 (port 1..65535, timeout-ms > 0, unit-id 1..247).WriteCommandRegionValidationTests— read-only region rejection (DiscreteInputs / InputRegisters) and the Coils-non-Bool guard for finding 002.ProbeCommandTests— the newComputeVerdicthelper for finding 006 (OK / DEGRADED / FAIL grid).CommandCancellationTests— Ctrl+C-during-initialize forProbeCommand/ReadCommand/WriteCommandfor 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.