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.5 KiB
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.mdthe 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-testedFocasDriver; the only CLI-local logic isParseValue/ParseBool/SynthesiseTagName, which the sibling CLIs cover with unit tests. The absence of a*.Cli.Testsproject 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)