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>
12 KiB
Code Review — Client.CLI
| Field | Value |
|---|---|
| Module | src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | 76d35d1 |
| Status | Reviewed |
| Open findings | 10 |
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 | Client.CLI-001, Client.CLI-002, Client.CLI-003 |
| 2 | OtOpcUa conventions | Client.CLI-004 |
| 3 | Concurrency & thread safety | Client.CLI-005 |
| 4 | Error handling & resilience | Client.CLI-006 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | Client.CLI-007 |
| 7 | Design-document adherence | Client.CLI-008 |
| 8 | Code organization & conventions | Client.CLI-009 |
| 9 | Testing coverage | Client.CLI-010 |
| 10 | Documentation & comments | Client.CLI-008 |
Findings
Client.CLI-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | Commands/HistoryReadCommand.cs:73, Commands/HistoryReadCommand.cs:76 |
| Status | Open |
Description: The start and end options are parsed with DateTime.Parse(StartTime) with
no IFormatProvider or DateTimeStyles. Parsing therefore depends on the current OS
culture: the same --start "03/04/2026" resolves to March 4 on an en-US box and April 3
on an en-GB box. The CLI is documented as cross-platform and the value silently produces a
different (wrong) history window rather than failing. The doc claims "ISO 8601 or date
string" but ISO interpretation is not guaranteed without DateTimeStyles.RoundtripKind or
CultureInfo.InvariantCulture. A bare date string is also assumed to be local time, then
.ToUniversalTime() shifts it by the host offset, so the same input yields different
ranges on machines in different time zones.
Recommendation: Parse with CultureInfo.InvariantCulture and
DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal (or require explicit
ISO 8601 via DateTimeOffset.Parse), and document the expected format and timezone
assumption precisely.
Resolution: (open)
Client.CLI-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | Commands/SubscribeCommand.cs:129-137 |
| Status | Open |
Description: The summary computes neverWentBad as every target whose node-id key is
absent from the everBad dictionary. A node that received no update at all is also absent
from everBad, so it is counted in neverWentBad and printed under the heading
"--- Nodes that NEVER received a bad-quality update (suspect) ---". The same node is also
listed separately under never ("never received an update at all"). Labeling a node that
produced zero notifications as a "suspect that never went bad" is misleading — it has not
been observed at all, which is a different (and arguably worse) condition than a node that
streamed only good values.
Recommendation: Exclude no-update nodes from the neverWentBad set, e.g.
targets.Where(t => lastStatus.ContainsKey(key) && !everBad.ContainsKey(key)), so the
"suspect" list only contains nodes that were actually observed and never reported bad
quality.
Resolution: (open)
Client.CLI-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | Commands/BrowseCommand.cs:29-30, Commands/SubscribeCommand.cs:20-27, Commands/AlarmsCommand.cs:28-29, Commands/HistoryReadCommand.cs:42-43 |
| Status | Open |
Description: Numeric command options accept any value with no range validation.
--depth, --interval, --max-depth, --max, and the history --interval can all be
supplied as 0 or a negative number. A negative --depth/--max-depth silently disables
recursion or under-traverses; a zero/negative sampling --interval is passed straight
through to SubscribeAsync and depends on the SDK/server to reject it; a negative --max
is forwarded to HistoryReadRawAsync. None of these produce a clear operator-facing error.
Recommendation: Validate option ranges at the start of ExecuteAsync and throw
CliFx.Exceptions.CommandException with an actionable message when a value is out of
range.
Resolution: (open)
Client.CLI-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | Commands/SubscribeCommand.cs:13-37 |
| Status | Open |
Description: SubscribeCommand is the only command in the module whose constructor
and all [CommandOption] properties have no XML doc comments. Every other command
(ConnectCommand, ReadCommand, WriteCommand, BrowseCommand, AlarmsCommand,
HistoryReadCommand, RedundancyCommand) and CommandBase carry <summary> docs on the
type, constructor, and options. The inconsistency is visible in IDE tooltips and breaks the
otherwise-uniform documentation convention of the module.
Recommendation: Add <summary> XML docs to the SubscribeCommand constructor and to
each of its option properties, matching the style used by the sibling commands.
Resolution: (open)
Client.CLI-005
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Concurrency & thread safety |
| Location | Commands/SubscribeCommand.cs:66-78, Commands/AlarmsCommand.cs:52-64 |
| Status | Open |
Description: The DataChanged and AlarmEvent handlers write to console.Output
(a System.IO.TextWriter) directly from the OPC UA SDK subscription/notification thread,
while the command main flow is awaiting Task.Delay(Timeout.Infinite, ct) and the summary
block also writes to the same console.Output. TextWriter instances are not guaranteed
thread-safe; concurrent WriteLine calls from the notification thread and the main thread
(a data-change notification arriving while the summary is being printed, or two
notifications from different SDK threads) can interleave or corrupt output. The handler
also calls the synchronous WriteLine and discards any exception, which on a fault would
propagate into the SDK callback.
Recommendation: Serialize console writes from event handlers — funnel notifications
through a Channel<T> drained by the main thread, or guard every console.Output write
with a shared lock. At minimum, ensure handler exceptions cannot escape into the SDK
callback.
Resolution: (open)
Client.CLI-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | Commands/HistoryReadCommand.cs:73, Commands/HistoryReadCommand.cs:76, Helpers/NodeIdParser.cs:39 |
| Status | Open |
Description: Operator input-format errors surface as raw .NET exceptions rather than
clean CLI errors. An unparseable start/end value throws FormatException straight out of
DateTime.Parse; an invalid node id throws FormatException/ArgumentException from
NodeIdParser. CliFx renders unhandled exceptions with a stack trace, which is noisy for a
user-input mistake. Other tooling in this module already distinguishes operator errors
(ParseAggregateType throws ArgumentException with a helpful message) but none of these
is converted to a CliFx.Exceptions.CommandException with a clean exit code.
Recommendation: Catch the predictable input-validation exceptions and rethrow as
CommandException with a concise message and a non-zero exit code, so malformed input
yields a one-line error instead of a stack trace.
Resolution: (open)
Client.CLI-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | CommandBase.cs:112-123 |
| Status | Open |
Description: ConfigureLogging builds a new Serilog LoggerConfiguration, creates a
logger, and assigns it to the static Log.Logger without disposing the previously
assigned logger. For a single CLI invocation this leaks at most one logger and the process
exits shortly after, so impact is minimal — but CommandBase is also exercised repeatedly
in-process by the unit-test suite, where each ExecuteAsync replaces Log.Logger and
abandons the prior console sink without disposal. The pattern is incorrect:
Log.CloseAndFlush() (or disposing the prior logger) should run before reassignment.
Recommendation: Call Log.CloseAndFlush() before assigning a new Log.Logger, or
build the logger into a local ILogger the command owns and disposes, rather than mutating
global static state per command.
Resolution: (open)
Client.CLI-008
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | docs/Client.CLI.md:158-217 |
| Status | Open |
Description: docs/Client.CLI.md is stale relative to the code at this commit.
(1) The subscribe command section documents only -n and -i, but the code
(SubscribeCommand) also exposes -r/--recursive, --max-depth, -q/--quiet,
--duration, and --summary-file — none are documented, and the documented Ctrl+C-only
lifecycle no longer matches --duration auto-exit.
(2) The historyread "Aggregate mapping" table lists six aggregates but the code
(HistoryReadCommand.ParseAggregateType and AggregateType) also supports
StandardDeviation (aliases stddev/stdev); the doc option table omits it while the
code option description includes it.
Recommendation: Regenerate the subscribe and historyread sections of
docs/Client.CLI.md from the current option set, including the five new subscribe flags
and the StandardDeviation aggregate row.
Resolution: (open)
Client.CLI-009
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | Commands/SubscribeCommand.cs:66-165, Commands/AlarmsCommand.cs:52-91 |
| Status | Open |
Description: Both long-running commands attach an event handler
(service.DataChanged += ..., service.AlarmEvent += ...) with a lambda and never detach
it. Because the handler closes over console, the captured console and the closure remain
referenced by the service until the service is disposed in the finally block. In
practice the service is per-command and disposed at the end, so this does not leak across
commands — but it is a latent footgun: a handler can still fire between UnsubscribeAsync
/ UnsubscribeAlarmsAsync and Dispose, writing to a console that the command considers
finished (overlapping with Client.CLI-005). The cleanup unsubscribes the monitored items
but never the .NET event.
Recommendation: Detach the handler explicitly (service.DataChanged -= handler) after
unsubscribing, using a named local delegate so it can be removed, ensuring no notification
is processed after the command output phase ends.
Resolution: (open)
Client.CLI-010
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs |
| Status | Open |
Description: The new SubscribeCommand capabilities are largely untested. The four
SubscribeCommandTests cover only single-node subscribe, unsubscribe-on-cancel,
disconnect-in-finally, and the subscription message. There is no test for the --recursive
browse-and-collect path (CollectVariablesAsync), the --duration auto-exit path, the
summary classification logic (good/bad/never/neverWentBad, including the
mislabeling noted in Client.CLI-002), the --quiet flag, the --summary-file write, or
per-node subscribe-failure handling. The summary logic is the most behaviour-rich part of
the command and the part most likely to regress.
Recommendation: Add unit tests for recursive variable collection, the duration-based
exit, summary bucketing across good/bad/no-update nodes, and the --summary-file output.
The FakeOpcUaClientService already exposes RaiseDataChanged, so feeding good/bad values
and asserting the summary text is straightforward.
Resolution: (open)