# Code Review — Driver.Cli.Common

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

## Findings

### Driver.Cli.Common-001

| Field | Value |
|---|---|
| Severity | High |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:106-119` |
| Status | Resolved |

**Description:** The `FormatStatus` shortlist maps four OPC UA status names to incorrect
numeric codes. The correct OPC UA spec values (verified against the OPC Foundation
UA-.NETStandard `Opc.Ua.StatusCodes` table) are:

| Name in shortlist | Code used | Correct code | What the used code actually is |
|---|---|---|---|
| `BadTimeout` | `0x80060000` | `0x800A0000` | `0x80060000` = `BadOutOfMemory` |
| `BadNoCommunication` | `0x80070000` | `0x80310000` | `0x80070000` = `BadResourceUnavailable` |
| `BadWaitingForInitialData` | `0x80080000` | `0x80320000` | `0x80080000` is not this name |
| `BadNodeIdInvalid` | `0x80350000` | `0x80330000` | `0x80350000` = `BadNodeClassInvalid` |

`Good` (`0x00000000`), `Bad` (`0x80000000`), `BadCommunicationError` (`0x80050000`),
`BadNodeIdUnknown` (`0x80340000`), `BadTypeMismatch` (`0x80740000`), and `Uncertain`
(`0x40000000`) are correct.

This is operator-facing and load-bearing: the CLI whole purpose is to label driver
status codes so a human can interpret a probe/read/write. A real device timeout
(`0x800A0000`) renders as bare `0x800A0000` with no name, while an out-of-memory
status (`0x80060000`) is mislabeled `BadTimeout`. A driver returning
`BadNodeClassInvalid` (`0x80350000`) is mislabeled `BadNodeIdInvalid`. The
`SnapshotFormatterTests` `[Theory]` cases for these codes assert against the wrong
expectations and therefore pass while the mapping is wrong (see Driver.Cli.Common-005).

**Recommendation:** Correct the four mappings to the spec values. Prefer deriving names
from the OPC Foundation `Opc.Ua.StatusCodes` constants (the stack the project already
depends on transitively) rather than hand-maintaining a hex shortlist, so the table
cannot drift from the spec again. If a hand-list is kept, add a test that cross-checks
each entry against `Opc.Ua.StatusCodes` reflection.

**Resolution:** Resolved 2026-05-22 — corrected the four mismapped `FormatStatus` codes
to their canonical `Opc.Ua.StatusCodes` values (`BadTimeout` 0x800A0000, `BadNoCommunication`
0x80310000, `BadWaitingForInitialData` 0x80320000, `BadNodeIdInvalid` 0x80330000); the CLI
project does not reference the `Opc.Ua` package so the hex literals were corrected in place
with a sync note, and `SnapshotFormatterTests` was updated with corrected expectations plus
a regression `[Theory]` asserting the pre-fix wrong names no longer apply.

### Driver.Cli.Common-002

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:101-122` |
| Status | Resolved |

**Description:** `FormatStatus` matches the full 32-bit status word for exact equality
against the shortlist. OPC UA status codes carry sub-code/flag bits in the low 16 bits
(info type, structure-changed, semantics-changed, limit bits, overflow, etc.). A
driver-supplied status such as `0x80050001` or any `Good` value with info bits set
(e.g. an overflow bit) falls through the `switch` and renders as bare hex even though
the high bits clearly identify the severity class. The doc comment on `FormatStatus`
claims the well-known statuses are named, but only the bit-exact canonical forms are.

**Recommendation:** Either (a) narrow the doc-comment claim to bit-exact canonical
codes, or (b) match on the severity bits (`code & 0xC0000000`) to at least always emit
`Good` / `Uncertain` / `Bad` even when sub-code bits are set, and match the named codes
on the masked code (`code & 0xFFFF0000`).

**Resolution:** Resolved 2026-05-22 — `FormatStatus` now matches named codes on `code & 0xFFFF0000` and falls back to a severity-class label (`Good`/`Uncertain`/`Bad`) via `code & 0xC0000000` for unknown sub-codes; the stale "bare-hex for unknown codes" test expectation was corrected to reflect the new severity-class fallback.

### Driver.Cli.Common-003

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
| Status | Resolved |

**Description:** `ConfigureLogging` assigns the process-global `Serilog.Log.Logger`
without disposing the previously assigned logger and the library never calls
`Log.CloseAndFlush()`. Each call creates a fresh `Logger` via `CreateLogger()` and
overwrites `Log.Logger`; the prior instance (and its console sink) is never disposed
or flushed. The class is the shared base for every driver CLI and the `subscribe` verb
is long-running — if any command path re-invokes `ConfigureLogging` the buffered
console sink is abandoned without a flush, and on process exit the final logger is also
never flushed. Verbose debug output written just before exit can be lost.

**Recommendation:** Call `Log.CloseAndFlush()` on shutdown (e.g. in a `finally` in the
command `ExecuteAsync`, or via a `protected` disposal helper on this base). Treat
`ConfigureLogging` as call-once / idempotent and document that. At minimum capture and
dispose the previous logger if reconfiguration is genuinely intended.

**Resolution:** Resolved 2026-05-22 — `ConfigureLogging` is now idempotent (guarded by `_loggingConfigured` field) and disposes the previous `Log.Logger` before overwriting; a new `protected static FlushLogging()` helper calls `Log.CloseAndFlush()` for commands to call in their `finally` blocks; XML doc updated accordingly.

### Driver.Cli.Common-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
| Status | Resolved |

**Description:** `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the
value and status columns) without guarding against empty input. When `tagNames` and
`snapshots` are both empty (equal length, so the mismatch check at line 56 passes),
`Enumerable.Max` throws `InvalidOperationException` ("Sequence contains no elements").
A batch read that legitimately returns zero tags therefore crashes the formatter
instead of producing an empty (header-only) table.

**Recommendation:** Short-circuit on `rows.Length == 0` (return just the header +
separator, or an explicit "no rows" line), or use `DefaultIfEmpty(0).Max(...)` for the
width computations.

**Resolution:** Resolved 2026-05-23 — `FormatTable` guards each `rows.Max(...)` width
computation with a `rows.Length == 0 ? "<HEADER>".Length : Math.Max(...)` ternary, so
an empty batch read returns the header + separator rows (no data rows) instead of
throwing `InvalidOperationException`. The fix was landed in commit `1433a1c` alongside
the -002 work, and the regression test
`SnapshotFormatterTests.FormatTable_with_empty_input_returns_header_only` (added under
-005) exercises it.

### Driver.Cli.Common-005

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:27-37` |
| Status | Resolved |

**Description:** The `FormatStatus_names_well_known_status_codes` `[Theory]` asserts
`0x80060000 => "BadTimeout"`, which encodes the wrong spec value (see
Driver.Cli.Common-001). The test passes because it validates the formatter against the
same incorrect table, so the bug is invisible to CI. Additionally there is no coverage
for: `DriverCommandBase` (`ConfigureLogging` verbose vs non-verbose level selection — no
test exercises the base at all), `FormatTable` with empty input (Driver.Cli.Common-004
would have been caught), `FormatValue` with array / enum / custom `object` values, and
`FormatTimestamp` with `DateTimeKind.Unspecified` (the docs imply Unspecified is
normalised but only `Local` is tested).

**Recommendation:** Fix the `[Theory]` expectations once Driver.Cli.Common-001 is
resolved, and add a test asserting each shortlist entry against the OPC Foundation
`Opc.Ua.StatusCodes` constants so the table cannot silently drift. Add `FormatTable`
empty-input and `DriverCommandBase` level-selection tests.

**Resolution:** Resolved 2026-05-22 — added `FormatTable_with_empty_input_returns_header_only` (exercises the -004 fix), `FormatStatus_with_sub_code_bits_resolves_to_named_class` / `FormatStatus_unknown_sub_code_falls_back_to_severity_class` Theories (cover -002 fix), and a new `DriverCommandBaseTests` class with four tests covering verbose/non-verbose level selection, idempotency of `ConfigureLogging`, and `FlushLogging`; stale `FormatStatus_unknown_codes_fall_back_to_hex_only` expectation corrected to match the -002 severity-class fallback.

### Driver.Cli.Common-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` |
| Status | Resolved |

**Description:** Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71`
states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement
needed" — true only when every snapshot has a non-null `SourceTimestampUtc`.
`FormatTimestamp` returns `"-"` for a null timestamp, so a mixed table has a 1-char-wide
cell in an otherwise 24-char column; the column is unaligned. Harmless (right-most, no
padding consumer) but the stated invariant does not hold. (2) The `DriverCommandBase`
class summary enumerates "Modbus / AB CIP / AB Legacy / S7 / TwinCAT" as the driver CLIs
but omits FOCAS, which `docs/DriverClis.md` lists as the sixth CLI built on this shared
library. The XML doc is stale relative to the shipped driver-CLI set.

**Recommendation:** Reword the `SnapshotFormatter.cs:71` comment to note the column is
right-most and intentionally unpadded rather than claiming fixed width. Add FOCAS to the
`DriverCommandBase` class-summary driver list.

**Resolution:** Resolved 2026-05-23 — (1) `SnapshotFormatter.cs:71` comment reworded
to state the source-time column is the right-most one and intentionally not
measured/padded, calling out the null-timestamp `"-"` case explicitly. (2) FOCAS was
added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff356b`
(landed alongside the -003 work).