Files

Joseph Doherty f46e126208 fix(driver-ablegacy-cli): resolve Low code-review findings (Driver.AbLegacy.Cli-002,003,004,005,006,007)

- Driver.AbLegacy.Cli-002: WriteCommand.Value description lists the full
  true/false, 1/0, on/off, yes/no alias set.
- Driver.AbLegacy.Cli-003: SubscribeCommand serialises every WriteLine
  via a per-execution consoleGate lock so the poll-thread OnDataChange
  handler can't interleave with the banner.
- Driver.AbLegacy.Cli-004: dropped 'await using var driver' in favour of
  a plain 'var driver' + explicit await ShutdownAsync in finally; the
  driver is no longer shut down twice.
- Driver.AbLegacy.Cli-005: SubscribeCommand.IntervalMs description
  carries the PollGroupEngine 250ms-floor caveat; docs/Driver.AbLegacy.Cli.md
  spells out the same.
- Driver.AbLegacy.Cli-006: ProbeCommand --type now carries the short
  alias 't' to match the other commands.
- Driver.AbLegacy.Cli-007: BuildOptionsTests cover the probe-disabled,
  device-shape, tag-passthrough, timeout-propagation, and empty-tag-list
  paths.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

2026-05-23 08:34:32 -04:00

12 KiB

Raw Blame History

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	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.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.

12 KiB Raw Blame History