8568f5cd85
Reviewed all 31 src/ production projects against the 10-category checklist in REVIEW-PROCESS.md. Each module gets its own findings.md; code-reviews/README.md is regenerated from them. 334 findings: 6 Critical, 46 High, 126 Medium, 156 Low. Critical findings: - Server-001: WriteNodeIdUnknown recurses unconditionally — a HistoryRead on an unresolvable node crashes the process (remote DoS). - Admin-001/002: app-wide auth bypass (RouteView not AuthorizeRouteView) plus unauthenticated mutating routes. - Core.Scripting-001: System.Environment reachable from operator scripts; Environment.Exit() terminates the server. - Core.AlarmHistorian-001: rowIds/events parallel-list desync on a corrupt payload misapplies outcomes — silent alarm-event data loss. - Driver.Galaxy-001: ReconnectSupervisor is built but never triggered, so a transient gateway drop permanently kills the event stream. All findings are Status=Open; resolution is tracked per REVIEW-PROCESS.md section 4. Review only — no source code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8.9 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 | 6 |
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 | Open |
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: (open)
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 | Open |
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: (open)
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)