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

# Code Review — Driver.AbCip.Cli

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

## 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.AbCip.Cli-001, Driver.AbCip.Cli-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.AbCip.Cli-003 |
| 4 | Error handling & resilience | Driver.AbCip.Cli-001, Driver.AbCip.Cli-004 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | Driver.AbCip.Cli-005 |
| 7 | Design-document adherence | Driver.AbCip.Cli-006 |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | Driver.AbCip.Cli-007 |
| 10 | Documentation & comments | Driver.AbCip.Cli-008 |

## Findings

<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
     never reused. Findings are never deleted — close them by changing Status and
     completing Resolution. -->

### Driver.AbCip.Cli-001

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs:70-85` |
| Status | Resolved |

**Description:** `ParseValue` parses every numeric Logix type with the BCL `*.Parse`
methods (`sbyte.Parse`, `short.Parse`, `int.Parse`, `float.Parse`, ...). These throw
the raw `FormatException` and `OverflowException` on bad operator input. The module's
own test `ParseValue_non_numeric_for_numeric_types_throws` confirms a raw
`FormatException` escapes for `DInt`. Meanwhile the `Bool` branch and the `_ =>`
default branch throw the CLI-friendly `CliFx.Exceptions.CommandException` with an
actionable message. The result is inconsistent operator UX: a typo in a boolean
value prints "Boolean value 'x' is not recognised...", but a typo in a numeric
value (`write -v 12x --type DInt`, or an out-of-range `write -v 99999999999 --type
Int`) escapes uncaught and CliFx renders a full .NET stack trace instead of a
one-line error. CliFx only formats `CommandException` cleanly.

**Recommendation:** Wrap the numeric `*.Parse` calls (or the whole `switch`) in a
`try`/`catch (Exception ex) when (ex is FormatException or OverflowException)` that
rethrows as a `CommandException` with the raw value, the target `--type`, and the
valid range — mirroring the `ParseBool` failure message.

**Resolution:** Resolved 2026-05-22 — wrapped the `ParseValue` switch in `try/catch (FormatException or OverflowException)` that rethrows as `CommandException` with the raw value and type; updated the previously-passing `ParseValue_non_numeric_for_numeric_types_throws` test to assert `CommandException` and added two new tests covering overflow and actionable message content.

### Driver.AbCip.Cli-002

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs:21-23`; `Commands/ReadCommand.cs:24-25`; `Commands/SubscribeCommand.cs:20-22` |
| Status | Resolved |

**Description:** `ProbeCommand`, `ReadCommand`, and `SubscribeCommand` expose
`--type` as a free `AbCipDataType` enum option with no exclusion of
`AbCipDataType.Structure`. Only `WriteCommand` rejects `Structure` (with an explicit
`CommandException`). Passing `probe/read/subscribe --type Structure` synthesises a
tag with `DataType = Structure` and no `Members` declared. The driver read path
treats a memberless Structure tag as a black box and routes it to the per-tag
fallback, where `runtime.DecodeValue(AbCipDataType.Structure, ...)` runs with no
declared layout — the operator gets either an opaque value or a confusing status
code rather than the clean "Structure writes need an explicit member layout"
guidance `write` gives. The `read` doc comment even claims "UDT / Structure reads
are out of scope here", but the code does not enforce it.

**Recommendation:** Reject `AbCipDataType.Structure` in `ProbeCommand`,
`ReadCommand`, and `SubscribeCommand` `ExecuteAsync` with the same `CommandException`
pattern `WriteCommand` uses, or factor a shared `RejectStructure(DataType)` guard
into `AbCipCommandBase`.

**Resolution:** Resolved 2026-05-22 — added `RejectStructure(AbCipDataType)` static helper to `AbCipCommandBase` that throws `CommandException` for `Structure`; called at the top of `ExecuteAsync` in `ProbeCommand`, `ReadCommand`, and `SubscribeCommand`.

### Driver.AbCip.Cli-003

| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` |
| Status | Resolved |

**Description:** The `OnDataChange` handler writes change lines to `console.Output`
(a `TextWriter`) from the driver's poll-engine callback thread, while the command's
main flow concurrently writes the "Subscribed to ... Ctrl+C to stop." line on the
CLI thread. `TextWriter.WriteLine` is not guaranteed thread-safe; concurrent writes
from the poll thread and the main thread can interleave or, in the worst case,
corrupt buffered output. The window is small (one main-thread write right after
`SubscribeAsync`) but it exists, and any future addition of main-thread output
during the watch loop widens it.

**Recommendation:** Emit the "Subscribed..." banner before registering the
`OnDataChange` handler (or before `SubscribeAsync`), or guard all `console.Output`
writes during the subscription with a shared lock so poll-thread and main-thread
output cannot interleave.

**Resolution:** Resolved 2026-05-23 — moved the "Subscribed to ... Ctrl+C to stop."
banner write to BEFORE `driver.OnDataChange +=` (and therefore before
`SubscribeAsync`). With the handler not yet attached when the banner runs, the
poll thread cannot fire change events into `console.Output` concurrently with the
main-thread banner write. After `+=` the only writer to `console.Output` is the
poll-thread handler, so no interleaving is possible.

### Driver.AbCip.Cli-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` |
| Status | Resolved |

**Description:** `--interval-ms` (`IntervalMs`) is taken verbatim and passed as
`TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A
zero or negative value produces a non-positive `TimeSpan`; the option description
claims "PollGroupEngine floors sub-250ms values" but says nothing about `0` or
negatives, and the flooring behaviour is the engine's, not the CLI's — relying on a
downstream component to sanitise operator input is fragile. `--timeout-ms` on
`AbCipCommandBase` has the same gap (a negative value yields a negative `TimeSpan`).

**Recommendation:** Validate `IntervalMs > 0` and `TimeoutMs > 0` at the top of
`ExecuteAsync` / in `AbCipCommandBase`, throwing a `CommandException` with the
accepted range when out of bounds.

**Resolution:** Resolved 2026-05-23 — added `SubscribeCommand.ValidateInterval(int)`
(throws `CommandException` for `IntervalMs <= 0`) called at the top of
`SubscribeCommand.ExecuteAsync`; moved `TimeoutMs > 0` validation into the
`AbCipCommandBase.Timeout` getter (throws `CommandException` for non-positive
`TimeoutMs`) and added a `_ = Timeout` touch in `SubscribeCommand.ExecuteAsync` to
fire that guard before the driver opens. Other commands trip the same guard
naturally via `BuildOptions(...).Timeout`. Regression tests
`AbCipCommandBaseTests.Timeout_get_throws_CommandException_when_TimeoutMs_is_non_positive`
and `SubscribeCommandIntervalTests.ValidateInterval_rejects_non_positive` cover
both paths.

### Driver.AbCip.Cli-005

| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
| Status | Resolved |

**Description:** `ConfigureLogging` assigns a freshly created Serilog logger to the
process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived
one-shot command (`probe`, `read`, `write`) the process exit flushes the console
sink, so the practical impact is nil. For `subscribe` — a long-running command
terminated by Ctrl+C — buffered log lines emitted just before cancellation can be
lost on abrupt exit. (This lives in the shared `Driver.Cli.Common` base, so it is
noted here as it affects the AB CIP CLI; the canonical fix belongs in that shared
module's review.)

**Recommendation:** Register `Log.CloseAndFlush()` on process exit (e.g. via
`AppDomain.ProcessExit` or a `finally` in the command), or have the CLI use a
disposable logger scoped to `ExecuteAsync`.

**Resolution:** Resolved 2026-05-23 — `Driver.Cli.Common` already exposes
`DriverCommandBase.FlushLogging()` (a `Log.CloseAndFlush()` wrapper); the AB CIP
CLI was not calling it. Added `FlushLogging()` in the `finally` block of all four
commands (`ProbeCommand`, `ReadCommand`, `WriteCommand`, `SubscribeCommand`) so
buffered Serilog output is flushed before the process exits, including the
Ctrl+C-driven `subscribe` path. No edits to `Driver.Cli.Common` were needed.

### Driver.AbCip.Cli-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` |
| Status | Resolved |

**Description:** `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout`
property with a getter derived from `TimeoutMs` and an empty `init` body
(`init { /* driven by TimeoutMs */ }`). Because the override has no
`[CommandOption]` attribute, CliFx never binds it, so the empty `init` is unreachable
in normal CLI use. However, an empty `init` accessor silently discards any
assignment — if a future caller or test constructs the command via an object
initializer (`new ReadCommand { Timeout = ... }`) the assignment is silently dropped
with no compiler warning. This is a latent correctness trap rather than a current
bug.

**Recommendation:** Either drop the `init` accessor entirely (make the override a
get-only expression-bodied property) or have the empty `init` throw
`NotSupportedException` to make the "driven by TimeoutMs" contract explicit and
fail-fast. (Drop is not viable because the abstract base declares `{ get; init; }`
and an override must provide both accessors.)

**Resolution:** Resolved 2026-05-23 — the `init` accessor now throws
`NotSupportedException` with a message pointing the caller at `TimeoutMs`. A new
test `AbCipCommandBaseTests.Timeout_setter_is_inert_and_does_not_silently_swallow_assignments`
asserts that an object-initializer assignment to `Timeout` fails fast.

### Driver.AbCip.Cli-007

| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` |
| Status | Resolved |

**Description:** The only test file covers `WriteCommand.ParseValue` and
`ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for
`AbCipCommandBase.BuildOptions` (the flag-to-`AbCipDriverOptions` mapping that all
four commands depend on) or `DriverInstanceId`. `BuildOptions` is pure and trivially
unit-testable yet untested: a regression that, say, flipped `EnableAlarmProjection`
back on or dropped `Probe.Enabled = false` would not be caught — and the comment
explicitly warns the probe loop "would race the operator's own reads", so that
mapping is behaviourally load-bearing. The `ExecuteAsync` bodies are reasonably left
untested (they need a fake `AbCipDriver` or hardware), consistent with the other
driver CLIs.

**Recommendation:** Add unit tests asserting `BuildOptions` produces
`Probe.Enabled == false`, `EnableControllerBrowse == false`,
`EnableAlarmProjection == false`, the expected single `AbCipDeviceOptions`
(`HostAddress`, `PlcFamily`, `DeviceName`), the supplied tag list, and the `Timeout`
derived from `TimeoutMs`.

**Resolution:** Resolved 2026-05-23 — added
`tests/.../AbCipCommandBaseTests.cs` covering `BuildOptions` (probe disabled,
controller-browse disabled, alarm-projection disabled, single device with
`HostAddress` / `PlcFamily` / `cli-{Family}` device name, tag list passed verbatim,
`Timeout` derived from `TimeoutMs`) and `DriverInstanceId` (`abcip-cli-{Gateway}`),
plus the `RejectStructure` guard (throws for `Structure`, no-op for atomic types).

### Driver.AbCip.Cli-008

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `docs/Driver.AbCip.Cli.md:8-9` |
| Status | Resolved |

**Description:** `docs/Driver.AbCip.Cli.md` opens with "Second of four driver
test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four"
contradicts the chain that follows it (five names) and contradicts
`docs/DriverClis.md`, which documents six CLIs (Modbus, AB CIP, AB Legacy, S7,
TwinCAT, FOCAS). The FOCAS CLI shipped alongside the Tier-C work, so the AB CIP
doc's "four" and the truncated chain are both stale.

**Recommendation:** Update the sentence to "Second of six driver test-client CLIs"
and complete the chain (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS), or
drop the explicit count and link `docs/DriverClis.md` as the authoritative roster.

**Resolution:** Resolved 2026-05-23 — rewrote the lead paragraph to "Second of six
driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS)"
and added a link to `docs/DriverClis.md` as the authoritative roster.