1433a1cf30
FormatStatus now matches named codes against code & 0xFFFF0000 (high-word mask) rather than exact equality, so status codes carrying sub-code or flag bits in the low 16 bits (e.g. 0x80050001) still resolve to their named class. For codes not in the named shortlist a severity-class fallback using the top 2 bits always emits Good / Uncertain / Bad rather than bare hex. Updated the stale FormatStatus_unknown_codes_fall_back_to_hex_only test (its expectation became invalid once the severity-class fallback was added) and added new Theory cases exercising both the high-word matching and the severity-class fallback paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9.7 KiB
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 | 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.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 | Open |
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: (open)
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 | Open |
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: (open)
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 | Open |
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: (open)
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 | Open |
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: (open)