lmxopcua/code-reviews/Driver.Modbus.Cli/findings.md

# Code Review — Driver.Modbus.Cli

| Field | Value |
|---|---|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 6 |

## Checklist coverage

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 |

## 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 | Open |

**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:** _(open)_


### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### Driver.Modbus.Cli-008

| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` |
| Status | Open |

**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:** _(open)_