Joseph Doherty
b50fd6c34a
Re-review at 7286d320. -008 (Low): all four commands now FlushLogging() in finally (parity
with AbCip.Cli; subscribe could drop shutdown log lines) + IL-inspection test.
303 lines
15 KiB
Markdown
303 lines
15 KiB
Markdown
# Code Review — Driver.AbLegacy.Cli
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli` |
|
|
| Reviewer | Claude Code |
|
|
| Review date | 2026-06-19 |
|
|
| Commit reviewed | `111d6983` |
|
|
| Status | Reviewed |
|
|
| Open findings | 0 |
|
|
|
|
## Re-review 2026-06-19 (commit 111d6983)
|
|
|
|
Re-review at HEAD `111d6983`. No changes in this module since the last review commit `7286d320`. Sibling module Driver.Cli.Common received a fix (`1180b017`) but the `FlushLogging` omission was not back-ported to AbLegacy. Cross-module comparison against the AbCip CLI (which received Driver.AbCip.Cli-005 fix) revealed the single new finding below.
|
|
|
|
#### Changes since prior review
|
|
|
|
No source files in this module changed between `7286d320` and `111d6983`. The finding was surfaced by comparing the AbLegacy CLI commands against the AbCip CLI siblings, which had `FlushLogging()` calls added in a prior review cycle.
|
|
|
|
## 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 | No issues found |
|
|
| 2 | OtOpcUa conventions | No issues found |
|
|
| 3 | Concurrency & thread safety | No issues found |
|
|
| 4 | Error handling & resilience | Driver.AbLegacy.Cli-008 |
|
|
| 5 | Security | No issues found |
|
|
| 6 | Performance & resource management | Driver.AbLegacy.Cli-008 |
|
|
| 7 | Design-document adherence | No issues found |
|
|
| 8 | Code organization & conventions | No issues found |
|
|
| 9 | Testing coverage | No issues found |
|
|
| 10 | Documentation & comments | No issues found |
|
|
|
|
Prior review checklist (commit `76d35d1`, 2026-05-22):
|
|
|
|
| # | 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 | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — updated `WriteCommand.Value` description to
|
|
list the full alias set: "booleans accept true/false, 1/0, on/off, yes/no".
|
|
Regression test `CommandMetadataTests.WriteCommand_value_help_lists_full_boolean_alias_set`
|
|
asserts the description contains every alias group.
|
|
|
|
### Driver.AbLegacy.Cli-003
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Concurrency & thread safety |
|
|
| Location | `Commands/SubscribeCommand.cs:47-53` |
|
|
| Status | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — `SubscribeCommand` now serialises every
|
|
console write through a shared `consoleGate` lock: the poll-thread
|
|
`OnDataChange` callback and the command-thread "Subscribed to ..." line both take
|
|
the lock before calling `WriteLine`. Comment in the source documents the intent.
|
|
|
|
### 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 | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — replaced `await using var driver` with a
|
|
plain `var driver` in all four commands (`ProbeCommand`, `ReadCommand`,
|
|
`WriteCommand`, `SubscribeCommand`), keeping the explicit
|
|
`finally { await driver.ShutdownAsync(CancellationToken.None) }` as the single
|
|
teardown path. Comment in each command documents the intent so readers do not
|
|
have to verify idempotency.
|
|
|
|
### 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 | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — extended the `SubscribeCommand.IntervalMs`
|
|
help text to match AbCip ("Publishing interval in milliseconds (default 1000).
|
|
PollGroupEngine floors sub-250ms values.") and added a paragraph under the
|
|
`subscribe` section in `docs/Driver.AbLegacy.Cli.md` naming the `-i` /
|
|
`--interval-ms` long form, the 1000 ms default, and the 250 ms floor. Regression
|
|
test `CommandMetadataTests.SubscribeCommand_interval_ms_help_notes_PollGroupEngine_floor`
|
|
asserts the description mentions "250".
|
|
|
|
### Driver.AbLegacy.Cli-006
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Code organization & conventions |
|
|
| Location | `Commands/ProbeCommand.cs:20-22` |
|
|
| Status | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — added the `'t'` short alias to
|
|
`ProbeCommand.DataType`. Regression test
|
|
`CommandMetadataTests.ProbeCommand_type_has_short_alias_t` (plus the parity test
|
|
`Other_commands_keep_type_short_alias_t` for read/write/subscribe) asserts the
|
|
short alias is present on every command. The same omission still exists in the
|
|
AbCip CLI's `ProbeCommand` — flagged as a sibling sweep but out of scope for
|
|
this module.
|
|
|
|
### 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 | Resolved |
|
|
|
|
**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:** Resolved 2026-05-23 — added `BuildOptionsTests` (five tests:
|
|
probe disabled, device shape from Gateway+PlcType, tag passthrough, timeout
|
|
propagation, empty tag list) covering `AbLegacyCommandBase.BuildOptions` via a
|
|
nested `TestCommand` subclass annotated with `[Command]` to satisfy the CliFx
|
|
analyzer. The overflow path for `ParseValue` is already covered by
|
|
`WriteCommandParseValueTests.ParseValue_out_of_range_throws_CommandException`
|
|
(theory with `short.Parse` + `AnalogInt` overflow inputs), added when finding
|
|
Driver.AbLegacy.Cli-001 was resolved.
|
|
|
|
---
|
|
|
|
## Findings (re-review 2026-06-19)
|
|
|
|
### Driver.AbLegacy.Cli-008
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Error handling & resilience / Performance & resource management |
|
|
| Location | `Commands/ProbeCommand.cs:58-61`, `Commands/ReadCommand.cs:52-55`, `Commands/WriteCommand.cs:62-65`, `Commands/SubscribeCommand.cs:88-96` |
|
|
| Status | Resolved |
|
|
|
|
**Description:** All four commands call `ConfigureLogging()` at the top of
|
|
`ExecuteAsync` but none call `FlushLogging()` (`Log.CloseAndFlush()`) in their
|
|
`finally` block. The `DriverCommandBase` XML doc explicitly states "Call
|
|
`FlushLogging` in a `finally` block to ensure buffered output is flushed before
|
|
the process exits." For the one-shot commands (`probe`, `read`, `write`) the
|
|
console sink is flushed on process exit anyway, so the practical impact is nil
|
|
there. For `subscribe` — a long-running command terminated by Ctrl+C — buffered
|
|
Serilog lines emitted during `ShutdownAsync` can be silently dropped on abrupt
|
|
exit. The sibling AbCip CLI already received this fix (Driver.AbCip.Cli-005,
|
|
resolved 2026-06-19); AbLegacy was not updated in the same pass.
|
|
|
|
**Recommendation:** Add `FlushLogging();` at the end of each command's `finally`
|
|
block, after `ShutdownAsync`, mirroring the AbCip CLI fix.
|
|
|
|
**Resolution:** Resolved 2026-06-19 — added `FlushLogging()` in the `finally`
|
|
block of all four commands (`ProbeCommand`, `ReadCommand`, `WriteCommand`,
|
|
`SubscribeCommand`) with an explanatory comment referencing this finding ID.
|
|
Added IL-inspection theory test
|
|
`CommandMetadataTests.ExecuteAsync_calls_FlushLogging_in_state_machine` (4
|
|
theory cases) that walks each command's compiler-generated async state-machine
|
|
`MoveNext` IL to verify the `DriverCommandBase.FlushLogging` method token is
|
|
present; test was red before the fix, green after. All 36 tests pass.
|