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

Code Review — Driver.FOCAS.Cli

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

Category 9 note: per docs/DriverClis.md the FOCAS CLI deliberately ships with no CLI-level test project (hardware-gated, followed the Tier-C isolation work on task #220). The four command classes are thin pass-throughs to the already-tested FocasDriver; the only CLI-local logic is ParseValue / ParseBool / SynthesiseTagName, which the sibling CLIs cover with unit tests. The absence of a *.Cli.Tests project is an intentional, documented gap rather than a review finding — but see Driver.FOCAS.Cli-001 for the parse path that would benefit most from coverage.

Findings

Driver.FOCAS.Cli-001

Field	Value
Severity	Low
Category	Error handling & resilience
Location	Commands/WriteCommand.cs:58-68
Status	Resolved

Description: WriteCommand.ParseValue parses the numeric --value types (Byte/Int16/Int32/Float32/Float64) with sbyte.Parse / short.Parse / etc. These throw raw FormatException or OverflowException for malformed or out-of-range input. Only the Bit case and the unsupported-type case throw CliFx.Exceptions.CommandException. CliFx renders a CommandException as a clean one-line error, but an uncaught FormatException/OverflowException surfaces as a full .NET stack trace — a poor experience for an operator who simply mistyped a value (e.g. write -a R100 -t Int16 -v abc). The parse failure occurs before any driver work, so the redundant stack trace also obscures that the write never reached the CNC.

Recommendation: Wrap the numeric parses (e.g. via TryParse per type, or a try/catch that rethrows as CommandException) so malformed --value input produces a clean, actionable message naming the expected type and the rejected literal — consistent with how ParseBool already handles bad boolean input. The same pattern exists in the sibling S7 CLI; a shared helper in Driver.Cli.Common would fix both.

Resolution: Resolved 2026-05-23 — wrapped the ParseValue numeric switch in try/catch (FormatException) and try/catch (OverflowException) that rethrow as CliFx.Exceptions.CommandException with a message naming the --type and the offending value, mirroring the friendly text the Bit path already produced. Added WriteCommandParseValueTests with [Theory] cases covering non-numeric input across Byte/Int16/Int32/Float32/Float64, overflow edges (sbyte ±1, short max+1, > int.MaxValue), and an assertion that the exception message names both the type and the offending value. A shared Driver.Cli.Common helper is the cleaner long-term fix (cross-CLI duplication remains) but is left to the Driver.Cli.Common review per this module's edit scope.

Driver.FOCAS.Cli-002

Field	Value
Severity	Low
Category	Concurrency & thread safety
Location	Commands/SubscribeCommand.cs:45-51
Status	Resolved

Description: The subscribe command attaches an OnDataChange handler that calls the synchronous console.Output.WriteLine. OnDataChange is raised from the driver's PollGroupEngine tick thread, while the command's main flow writes the "Subscribed to ..." banner from the CliFx invocation thread. The CliFx IConsole.Output TextWriter is not documented as thread-safe; with a single poll group the change events are serialised, but the banner write at line 55-56 can interleave with the first poll-driven change line. The handler is also never detached from the event before driver disposal — benign here because the driver is disposed in the same finally, but it leaves a dangling subscription if the command is ever refactored to reuse the driver.

Recommendation: Write the "Subscribed" banner before wiring the OnDataChange handler (it is informational and ordering-sensitive), or guard console writes with a lock shared between the banner and the handler. Optionally detach the handler in the finally block before ShutdownAsync for symmetry with the handle teardown already present there.

Resolution: Resolved 2026-05-23 — introduced a writeLock shared between the OnDataChange handler and the banner write so the poll-engine background thread and the CliFx invocation thread can't interleave partial lines. Added an explanatory comment above the handler explaining the CliFx-IConsole rationale and the synchronous-on-background-thread design — mirroring the Modbus / S7 copies of this command. Also added a try/catch around the handler body so a transient stdout error cannot tear down the poll loop, and Serilog-warn-logs the swallowed exception. Added SubscribeCommandConsoleHandlerTests to guard the writeLock + CliFx-IConsole rationale against future copy-paste regressions.

Driver.FOCAS.Cli-003

Field	Value
Severity	Low
Category	Error handling & resilience
Location	FocasCommandBase.cs:19 (CncPort), FocasCommandBase.cs:27 (TimeoutMs), Commands/SubscribeCommand.cs:23 (IntervalMs)
Status	Resolved

Description: The numeric command options --cnc-port, --timeout-ms, and --interval-ms are accepted without range validation. A zero or negative --cnc-port produces an invalid focas://host:<n> string; --timeout-ms 0 yields a zero TimeSpan operation timeout; a zero/negative --interval-ms produces a non-positive publishingInterval passed straight into PollGroupEngine.Subscribe. Depending on the engine tolerance these surface either as an opaque downstream exception or as a tight-spinning poll loop rather than a clear "value must be positive" message at the CLI boundary.

Recommendation: Validate the three numeric options at the top of ExecuteAsync (or in FocasCommandBase) and throw a CliFx.Exceptions.CommandException when out of range — port in 1..65535, timeout and interval strictly positive. The same gap exists across the sibling driver CLIs, so a shared validation helper in Driver.Cli.Common is the cleaner fix.

Resolution: Resolved 2026-05-23 — added a protected ValidateOptions(int? intervalMs = null) helper on FocasCommandBase that rejects --cnc-port outside 1..65535, non-positive --timeout-ms, and non-positive --interval-ms (when the caller passes one) with a CliFx.Exceptions.CommandException naming the option and the rejected value. ProbeCommand / ReadCommand / WriteCommand call ValidateOptions() without an interval, SubscribeCommand calls ValidateOptions(IntervalMs). Added FocasCommandBaseValidationTests covering accept-defaults, reject out-of-range port (0, -1, 65536), reject non-positive timeout / interval, and skip-interval-when-omitted. A shared helper in Driver.Cli.Common is the cleaner cross-CLI fix and is recorded against that module's review.

Driver.FOCAS.Cli-004

Field	Value
Severity	Low
Category	Performance & resource management
Location	Commands/ProbeCommand.cs:37,54; Commands/ReadCommand.cs:37,46; Commands/WriteCommand.cs:45,54; Commands/SubscribeCommand.cs:39,73
Status	Resolved

Description: Every command declares await using var driver = new FocasDriver(...) and explicitly calls await driver.ShutdownAsync(CancellationToken.None) in the finally block. FocasDriver.DisposeAsync() itself calls ShutdownAsync, so shutdown runs twice per command invocation. FocasDriver.ShutdownAsync is idempotent (it clears _devices / _tagsByName, and the second pass iterates an empty collection), so there is no functional bug — but the redundant call is dead weight and obscures intent: a reader cannot tell whether the explicit ShutdownAsync or the await using is the real teardown.

Recommendation: Drop the explicit ShutdownAsync from the finally blocks and rely on await using for disposal, or drop await using and keep the explicit teardown — but not both. The same redundancy exists in the sibling CLIs.

Resolution: Resolved 2026-05-23 — dropped the explicit await driver.ShutdownAsync(CancellationToken.None) calls from the finally blocks of ProbeCommand, ReadCommand, WriteCommand, and SubscribeCommand; await using is now the sole driver-disposal mechanism per command (FocasDriver.DisposeAsync itself runs ShutdownAsync). The subscribe command keeps UnsubscribeAsync in its finally because that is a subscription-lifecycle concern, not driver disposal. Added CommandDisposalConventionsTests to guard the source-level convention against regression.

Driver.FOCAS.Cli-005

Field	Value
Severity	Low
Category	Design-document adherence
Location	Commands/WriteCommand.cs:50, Commands/ProbeCommand.cs:50 (via SnapshotFormatter.FormatStatus)
Status	Resolved

Description: docs/Driver.FOCAS.Cli.md documents BadDeviceFailure and BadCommunicationError as the key diagnostic signals an operator reads off probe / write output ("A BadCommunicationError means ... BadDeviceFailure after a successful connect means ..."). The FOCAS driver FocasStatusMapper also emits BadNotWritable (0x803B0000), BadOutOfRange (0x803C0000), BadNotSupported (0x803D0000), BadDeviceFailure (0x80550000), BadInternalError (0x80020000), and BadTimeout (0x800A0000). The shared SnapshotFormatter.FormatStatus shortlist only names Good, Bad, BadCommunicationError, BadTimeout (0x80060000 — note this is a different code than the mapper BadTimeout 0x800A0000), BadNoCommunication, BadWaitingForInitialData, BadNodeIdUnknown, BadNodeIdInvalid, BadTypeMismatch, and Uncertain. Consequently a FOCAS write to a non-writable address, a parameter-write rejected by the CNC, or a BadDeviceFailure session-setup rejection renders as a bare hex code (0x803B0000, 0x80550000, …) with no name — directly contradicting the documented workflow where the operator is told to read those status names.

Recommendation: Extend SnapshotFormatter.FormatStatus (in Driver.Cli.Common) to name the Bad* codes the native-protocol drivers actually emit — at minimum BadNotWritable, BadOutOfRange, BadNotSupported, BadDeviceFailure, BadInternalError, and the mapper BadTimeout (0x800A0000). The fix belongs in the shared library, but it is recorded here because the gap defeats this module documented probe/write diagnostic workflow; cross-reference the Driver.Cli.Common review.

Resolution: Resolved 2026-05-23 — the cross-CLI fix landed in Driver.Cli.Common: SnapshotFormatter.FormatStatus now names BadInternalError (0x80020000), BadNotWritable (0x803B0000), BadOutOfRange (0x803C0000), BadNotSupported (0x803D0000), and BadDeviceFailure (0x80550000) — the five codes the FOCAS / AbCip / AbLegacy native-protocol mappers all emit but the shortlist previously left unnamed (the canonical BadTimeout 0x800A0000 was already added under Driver.Cli.Common-001). FOCAS probe / write against a non-writable parameter, out-of-range address, unsupported function, busy device, or CNC-handle failure now renders with the named status the docs/Driver.FOCAS.Cli.md workflow promises, restoring parity between the docs and the shipped behaviour. Regression [Theory] FormatStatus_names_native_driver_emitted_codes added to SnapshotFormatterTests so the five names can't silently drop out of the shortlist again; the existing well-known shortlist [Theory] was extended with the same five entries to enforce the exact 0x... (Name) rendering. Suite now 47 green (was 42).