# 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 | 5 |

## 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 | Open |

**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:** _(open)_

### Driver.FOCAS.Cli-002

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

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_
