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

# Code Review — Driver.AbLegacy.Cli

| Field | Value |
|---|---|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.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.AbLegacy.Cli-001, Driver.AbLegacy.Cli-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.AbLegacy.Cli-003 |
| 4 | Error handling & resilience | Driver.AbLegacy.Cli-001, Driver.AbLegacy.Cli-004 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | Driver.AbLegacy.Cli-005 |
| 8 | Code organization & conventions | Driver.AbLegacy.Cli-006 |
| 9 | Testing coverage | Driver.AbLegacy.Cli-007 |
| 10 | Documentation & comments | Driver.AbLegacy.Cli-002, Driver.AbLegacy.Cli-005 |

## Findings

### Driver.AbLegacy.Cli-001

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `Commands/WriteCommand.cs:46`, `Commands/WriteCommand.cs:62-72` |
| Status | Resolved |

**Description:** `WriteCommand.ExecuteAsync` calls `ParseValue(Value, DataType)` at
line 46, *before* the `try` block and outside any catch. `ParseValue` uses
`short.Parse` / `int.Parse` / `float.Parse`, which throw `FormatException` on
malformed input (`-v abc`) and `OverflowException` on out-of-range input
(`-t Int -v 99999`). Only the `Bit` branch and the unsupported-type branch raise
the CliFx `CommandException` that the framework renders as a clean one-line error
with a non-zero exit code. For every numeric type a bad `--value` therefore
escapes as an unhandled `FormatException`/`OverflowException`, which CliFx prints
as a raw stack trace — an operator-hostile failure mode for a tool whose whole
purpose is ad-hoc operator use. The module own test
`ParseValue_non_numeric_for_numeric_types_throws` confirms the raw `FormatException`
leaks. The driver `WriteAsync` has dedicated catch arms for `FormatException`
(`BadTypeMismatch`) and `OverflowException` (`BadOutOfRange`), but the CLI never
reaches the driver because the parse happens first.

**Recommendation:** Wrap the numeric parses so a parse failure surfaces as a
`CliFx.Exceptions.CommandException` with a message naming the offending value and
type (mirroring the existing `Bit` and unsupported-type branches). Either catch
`FormatException`/`OverflowException` inside `ParseValue` and rethrow as
`CommandException`, or use `TryParse` and throw `CommandException` on failure.

**Resolution:** Resolved 2026-05-22 — wrapped numeric parses in `ParseValue` with `try/catch` for `FormatException`/`OverflowException`, rethrowing as `CommandException` with a message naming the offending value and type; updated test to assert `CommandException` and added overflow regression test.

### Driver.AbLegacy.Cli-002

| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` |
| Status | Open |

**Description:** The `--value` option help text states "booleans accept
true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message
also accept `on/off` and `yes/no`, and `DriverClis.md` documents the full
`true/false/1/0/yes/no/on/off` set as the shared CLI contract. The help text
under-documents the accepted aliases, so an operator reading `--help` will not
discover `on`/`off`/`yes`/`no`. Minor, but it makes the inline help inconsistent
with both the code and the design doc.

**Recommendation:** Extend the `--value` description to list the full alias set,
matching the wording used elsewhere (e.g. "booleans accept
true/false, 1/0, on/off, yes/no").

**Resolution:** _(open)_

### Driver.AbLegacy.Cli-003

| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `Commands/SubscribeCommand.cs:47-53` |
| Status | Open |

**Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)`
(the synchronous overload) directly from the `PollGroupEngine` poll thread. The
poll engine raises change events from a background timer/loop thread, so two
ticks that fire close together can interleave writes on the shared `TextWriter`.
`SnapshotFormatter` builds the whole line into a single string before the call,
so a line is unlikely to be torn mid-token, but there is no synchronisation
guaranteeing that the background-thread writes do not interleave with the
`await console.Output.WriteLineAsync(...)` "Subscribed to ..." line on the command
thread, nor with each other. This is the same pattern as the AbCip CLI, so it is
a shared low-severity issue, not unique to this module.

**Recommendation:** Serialise console writes from the event handler — e.g. funnel
change events through a `Channel<string>` drained by a single consumer task, or
guard the `WriteLine` with a lock. At minimum, document that the interleaving is
accepted because output is human-facing and line-buffered.

**Resolution:** _(open)_

### Driver.AbLegacy.Cli-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` |
| Status | Open |

**Description:** Every command does `await using var driver = new AbLegacyDriver(...)`
*and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver`
`DisposeAsync` itself calls `ShutdownAsync`, so the driver is shut down twice on the
normal path. `ShutdownAsync` is written to be idempotent (it clears `_devices` /
`_tagsByName` and re-enters cleanly on an empty state), so this is not a crash, but
the double teardown is redundant and slightly obscures intent — a reader has to
confirm idempotency to be sure it is safe. The `await using` already guarantees
cleanup on every exit path including exceptions.

**Recommendation:** Drop either the `await using` or the explicit
`finally { await driver.ShutdownAsync(...) }` in each command. Keeping the explicit
`finally` and using a plain `var driver` (no `await using`) is the clearer choice,
since the commands deliberately pass `CancellationToken.None` to shutdown so teardown
is not cut short by a cancelled `ct`.

**Resolution:** _(open)_

### Driver.AbLegacy.Cli-005

| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` |
| Status | Open |

**Description:** The subscribe command interval option is `--interval-ms`
(default 1000). `docs/Driver.AbLegacy.Cli.md` shows the subscribe example as
`otopcua-ablegacy-cli subscribe ... -i 500`, which works because of the short
alias `'i'`, but the doc never names the long form `--interval-ms` or states the
1000 ms default, while the equivalent AbCip CLI help text notes "PollGroupEngine
floors sub-250ms values". The AbLegacy `--interval-ms` description omits that
flooring caveat, so an operator passing `-i 100` against AbLegacy gets no warning
that the engine will floor it. The behaviour is identical (same `PollGroupEngine`)
but the documented contract drifts between the two CLIs.

**Recommendation:** Add the sub-250 ms flooring note to the AbLegacy
`--interval-ms` description for parity with the AbCip CLI, and mention the
`--interval-ms` long form + 1000 ms default in `docs/Driver.AbLegacy.Cli.md`.

**Resolution:** _(open)_

### Driver.AbLegacy.Cli-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Commands/ProbeCommand.cs:20-22` |
| Status | Open |

**Description:** `ProbeCommand` declares its `--type` option with no short alias,
while `ReadCommand`, `WriteCommand`, and `SubscribeCommand` all declare `--type`
with the short alias `'t'`. `ProbeCommand` also gives `--address` the alias `'a'`,
matching the other commands, so the `--type` omission is an inconsistency rather
than a deliberate design choice. An operator who learns `-t` on `read` will find
it silently rejected on `probe`.

**Recommendation:** Add the `'t'` short alias to `ProbeCommand` `--type` option
for consistency with the other three commands. (The AbCip CLI `ProbeCommand` has
the same omission, so a cross-CLI sweep is worthwhile.)

**Resolution:** _(open)_

### Driver.AbLegacy.Cli-007

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

**Description:** The only test file in the CLI test project covers
`WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that
are pure logic (testable without a device) are uncovered:
(1) `AbLegacyCommandBase.BuildOptions` — that it sets `Probe.Enabled = false`,
populates `Devices` from `Gateway`/`PlcType`, and forwards the tag list; a
regression here silently changes every command behaviour.
(2) the out-of-range numeric path for `ParseValue` (`short.Parse` overflow,
`int.Parse` overflow) — `ParseValue_non_numeric_for_numeric_types_throws` asserts
`FormatException` for non-numeric input but nothing asserts the overflow path,
which is exactly the path that escapes uncaught per finding
Driver.AbLegacy.Cli-001. `BuildOptions` is reachable via `InternalsVisibleTo`
(the test assembly is already granted access).

**Recommendation:** Add tests for `BuildOptions` (probe disabled, device shape,
tag passthrough) and an overflow-input test for `ParseValue` so the fix for
Driver.AbLegacy.Cli-001 is locked in by a regression test.

**Resolution:** _(open)_