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-06-19 |
| Commit reviewed | `7286d320` |
| 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).

## Re-review 2026-06-19 (commit 7286d320)

All five prior findings (001–005) remain Resolved. The test suite now has 52 tests
(48 pre-existing + 4 added in this re-review). Two new findings were identified.

#### Checklist coverage (re-review)

| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No new issues found |
| 2 | OtOpcUa conventions | No new issues found |
| 3 | Concurrency & thread safety | No new issues found |
| 4 | Error handling & resilience | No new issues found |
| 5 | Security | No new issues found |
| 6 | Performance & resource management | No new issues found |
| 7 | Design-document adherence | No new issues found |
| 8 | Code organization & conventions | Driver.FOCAS.Cli-006, Driver.FOCAS.Cli-007 |
| 9 | Testing coverage | No new issues found |
| 10 | Documentation & comments | Driver.FOCAS.Cli-007 |

### Driver.FOCAS.Cli-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Commands/ProbeCommand.cs`, `Commands/ReadCommand.cs`, `Commands/WriteCommand.cs`, `Commands/SubscribeCommand.cs` |
| Status | Resolved |

**Description:** `DriverCommandBase.ConfigureLogging` was extended between review
commits to document that callers must call `FlushLogging()` in a `finally` block
to ensure buffered Serilog output is flushed before process exit. Every sibling
driver CLI (Modbus, AbCip, AbLegacy, TwinCAT) honours this contract, but all four
FOCAS CLI commands were missing the call. With the Console sink (Serilog 6.0.0)
there is no actual data loss today because `System.Console.Out` auto-flushes;
however the gap is a maintenance hazard: if a file sink or an async sink is ever
added to the logger configuration, the omission becomes data-losing. The
inconsistency with sibling CLIs also makes the codebase misleading for reviewers.

**Recommendation:** Add `try/finally` wrappers in ProbeCommand, ReadCommand, and
WriteCommand (which had no finally blocks since the -004 cleanup) and extend
SubscribeCommand's existing finally block, calling `FlushLogging()` in each.
Add a convention test (parallel to `CommandDisposalConventionsTests`) that
scans source files and asserts the call is present, preventing silent regressions.

**Resolution:** Resolved 2026-06-19 — added `try/finally` wrappers with
`FlushLogging()` to `ProbeCommand`, `ReadCommand`, and `WriteCommand`; extended
SubscribeCommand's existing `finally` block with the same call. Added
`FlushLoggingConventionsTests` with a `[Theory]` over all four command files so
the call cannot be silently dropped in future refactors. Suite now 52 green (was 48).

### Driver.FOCAS.Cli-007

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `Commands/SubscribeCommand.cs:113-120` |
| Status | Resolved |

**Description:** The comment in the `SubscribeCommand.finally` block (added by
the -002 resolution) said "detach the OnDataChange handler before unsubscribe +
disposal" — but no `-=` operator call exists in the block. The comment's claim
was factually incorrect: handler cleanup is achieved by driver disposal (via
`await using`), which tears down the `PollGroupEngine` and prevents further
`OnDataChange` events; not by explicit unsubscription from the event. A
parenthetical note at the end of the original comment partially explained the
real mechanism, but the leading sentence was still misleading for future readers.

**Recommendation:** Reword the comment to accurately describe what the code does:
`UnsubscribeAsync` stops the poll-group ticker (a subscription lifecycle step);
the anonymous handler is never explicitly removed via `-=`; driver disposal via
`await using` is the real cleanup.

**Resolution:** Resolved 2026-06-19 — rewrote the comment to correctly describe
the two-step teardown: `UnsubscribeAsync` halts the poll ticker (subscription
lifecycle), then `await using` disposes the driver and tears down
`PollGroupEngine` (real cleanup). Removed the incorrect claim about `-=`
detachment. No behavioral change; documentation only.