Joseph Doherty
f8bf067243
Re-review at 7286d320. -008 (Medium): S7CommandBase.ValidateEndpoint (port range + timeout>0)
in all commands +tests. -009 clean OperationCanceledException handling; -010 FlushLogging()
in subscribe finally; -011 lock console writes in OnDataChange. -012 (Verdict headline) deferred.
19 KiB
Code Review — Driver.S7.Cli
| Field | Value |
|---|---|
| Module | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | 111d6983 |
| 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.S7.Cli-001, Driver.S7.Cli-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No issues found |
| 4 | Error handling & resilience | Driver.S7.Cli-001, Driver.S7.Cli-003 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | Driver.S7.Cli-004 |
| 7 | Design-document adherence | Driver.S7.Cli-002 |
| 8 | Code organization & conventions | Driver.S7.Cli-005 |
| 9 | Testing coverage | Driver.S7.Cli-006 |
| 10 | Documentation & comments | Driver.S7.Cli-002, Driver.S7.Cli-007 |
Findings
Driver.S7.Cli-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/WriteCommand.cs:65-80 |
| Status | Resolved |
Description: WriteCommand.ParseValue parses numeric and DateTime values with the
raw BCL parsers (short.Parse, float.Parse, DateTime.Parse, etc.). On malformed
input these throw FormatException / OverflowException, which are not
CliFx.Exceptions.CommandException. CliFx renders a CommandException as a clean
one-line error with a non-zero exit code, but renders any other exception as a full
.NET stack trace. The ParseValue bool path is handled correctly (it throws
CommandException for unrecognised input), so the command is internally inconsistent:
write -t Bool -v maybe gives a friendly message while write -t Int16 -v xyz dumps a
stack trace. The module own test ParseValue_non_numeric_for_numeric_types_throws
asserts the raw FormatException leaks, confirming the behaviour is unintended-but-shipped.
Recommendation: Wrap the numeric / DateTime parses in a try/catch that
re-throws FormatException and OverflowException as
CliFx.Exceptions.CommandException with a message that names the --type and the
offending value — matching the bool path. Update the test to expect CommandException.
Resolution: Resolved 2026-05-22 — wrapped all numeric/DateTime BCL parses in try/catch(FormatException) and try/catch(OverflowException) that re-throw as CommandException with a message naming the --type and the offending value; updated ParseValue_non_numeric_for_numeric_types_throws to assert CommandException, and added an overflow-edge test.
Driver.S7.Cli-002
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Design-document adherence |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ReadCommand.cs:22-29, Commands/WriteCommand.cs:21-33, Commands/SubscribeCommand.cs:18-21; docs/Driver.S7.Cli.md:70-73,80-81 |
| Status | Resolved |
Description: The --type option help text on read, write, and subscribe
advertises the full S7DataType set (Int64 / UInt64 / Float64 / String / DateTime),
and docs/Driver.S7.Cli.md shows a worked read ... -t String --string-length 80
example plus a --string-length flag on read/write. The underlying S7Driver
(S7Driver.cs:241-245 for reads, :316-320 for writes) throws NotSupportedException
for Int64, UInt64, Float64, String, and DateTime — the driver maps that to
BadNotSupported. Consequently every CLI invocation using one of those types — and the
documented --string-length string-read example — fails at runtime with
0x803D0000 (Bad). The CLI surface and docs promise capability the driver does not yet
implement.
Recommendation: Either (a) trim the --type help text and the --string-length
flag/examples to the implemented set (Bool / Byte / Int16 / UInt16 / Int32 / UInt32 / Float32) until the follow-up driver PR lands, or (b) keep the surface but add a one-line
"types beyond Float32 are not yet implemented and surface BadNotSupported" caveat to the
help text and docs/Driver.S7.Cli.md. Option (a) is preferred so the CLI does not offer
options that cannot succeed.
Resolution: Resolved 2026-05-22 — updated the --type help text on read, write, and subscribe to list the implemented set (Bool/Byte/Int16/UInt16/Int32/UInt32/Float32) and appended a one-line caveat that Int64/UInt64/Float64/String/DateTime are not yet implemented and will return BadNotSupported.
Driver.S7.Cli-003
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:38-50 |
| Status | Resolved |
Description: ProbeCommand XML doc and the Driver.S7.Cli.md "fastest is the
device talking" framing say the probe "connects ... prints health" and "surfaces
BadNotSupported" when PUT/GET is disabled. But when the PLC is unreachable (connection
refused, host down, wrong slot), driver.InitializeAsync throws and the exception
propagates straight out of ExecuteAsync — the code that prints Host:, Health:,
Last error:, and the snapshot is never reached. The most common probe failure (device
not reachable at all) therefore produces a CliFx stack trace rather than the structured
health report the command exists to give. Note PUT/GET-disabled only surfaces during
ReadAsync (after a successful connect), so that one path does reach the health print —
but a refused TCP connect does not.
Recommendation: Wrap the InitializeAsync + ReadAsync body in a try/catch that,
on failure, still prints the Host: / CPU: lines and a Health: / Last error:
report derived from driver.GetHealth() (which InitializeAsync sets to
Faulted with the exception message before re-throwing). The probe should report an
unreachable device, not crash on it.
Resolution: Resolved 2026-05-22 — wrapped the InitializeAsync + ReadAsync body in a try/catch that on any non-cancellation failure still prints the structured Host:, CPU:, Health:, and Last error: lines derived from driver.GetHealth(), so an unreachable device produces a health report rather than a stack trace.
Driver.S7.Cli-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:36,53, Commands/ReadCommand.cs:45,54, Commands/WriteCommand.cs:51,60, Commands/SubscribeCommand.cs:39,73 |
| Status | Resolved |
Description: Every command declares the driver with await using var driver = new S7Driver(...) and also calls await driver.ShutdownAsync(...) in a finally block.
S7Driver.DisposeAsync itself calls ShutdownAsync, so shutdown runs twice per command
(three times for subscribe, which also unsubscribes). ShutdownAsync is idempotent
(Plc?.Close() is best-effort, _subscriptions is cleared) so there is no functional
bug, but the explicit finally-block ShutdownAsync call is redundant given the
await using. It is also slightly misleading — a reader may assume the await using is
not actually disposing.
Recommendation: Drop the explicit await driver.ShutdownAsync(...) from the
finally blocks and rely on await using for teardown; keep only the
subscribe command UnsubscribeAsync. Alternatively drop await using
and keep the explicit finally. Pick one disposal mechanism per command.
Resolution: Resolved 2026-05-23 — dropped the explicit await driver.ShutdownAsync(CancellationToken.None) calls from the finally blocks of ProbeCommand, ReadCommand, WriteCommand, and SubscribeCommand; await using is now the sole driver-disposal mechanism per command (DisposeAsync itself runs ShutdownAsync), and the subscribe command keeps UnsubscribeAsync in its finally because that is a subscription-lifecycle concern, not driver disposal. Added CommandDisposalConventionsTests to guard the source-level convention against regression.
Driver.S7.Cli-005
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ |
| Status | Resolved |
Description: A stale directory tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/
exists containing only an obj/ folder — no .csproj, no source. The real test
project lives at tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/. The empty
directory is a leftover from the project move into tests/Drivers/Cli/ and is not
referenced by ZB.MOM.WW.OtOpcUa.slnx. It is dead clutter that can mislead anyone
grepping the tree for the S7 CLI test project.
Recommendation: Delete the stale tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/
directory (including its obj/). This is outside the module src/ tree but is the
S7 CLI own orphaned test folder, so it belongs to this module cleanup.
Resolution: Resolved 2026-05-23 — deleted the stale tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ directory (only contained a leftover obj/ from before the move into tests/Drivers/Cli/; no tracked files). The real test project at tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ is untouched.
Driver.S7.Cli-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs |
| Status | Resolved |
Description: The only test file covers WriteCommand.ParseValue and
ReadCommand.SynthesiseTagName. S7CommandBase.BuildOptions — which maps the
host / port / CPU / rack / slot / timeout flags onto an S7DriverOptions and forces
Probe.Enabled = false — has no test, despite being pure, deterministic, and
internal-visible to the test assembly via InternalsVisibleTo. A regression that
dropped Probe = new S7ProbeOptions { Enabled = false } (which would start an
unwanted background probe loop in a one-shot CLI run) or mis-mapped TimeoutMs would
not be caught. ParseValue is also missing an explicit overflow-edge test (e.g.
Byte value 256) — the current ParseValue_Byte_ranges test stops at 255.
Recommendation: Add a BuildOptions test (assert Probe.Enabled == false,
Timeout matches TimeoutMs, and host/port/CPU/rack/slot flow through). Add an
overflow case to the ParseValue numeric tests once Driver.S7.Cli-001 is resolved so
the test asserts the wrapped CommandException.
Resolution: Resolved 2026-05-23 — added S7CommandBaseBuildOptionsTests covering Probe.Enabled=false (one-shot CLI guarantee), TimeoutMs→Timeout TimeSpan mapping, host/port/CPU/rack/slot flowing through, and tag-list passthrough. The overflow-edge ParseValue test was already added under Driver.S7.Cli-001 (ParseValue_overflow_for_numeric_types_throws_CommandException).
Driver.S7.Cli-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:45-51 |
| Status | Resolved |
Description: The Modbus CLI SubscribeCommand carries an explanatory comment on
the OnDataChange handler ("Route every data-change event to the CliFx console (not
System.Console — the analyzer flags it + IConsole is the testable abstraction)"). The S7
SubscribeCommand is a near-verbatim copy but dropped that comment, so the non-obvious
reason the handler uses console.Output.WriteLine (synchronous, on a driver background
thread) instead of System.Console or the async WriteLineAsync is undocumented here.
Minor, but the rationale is worth keeping consistent across the CLI family.
Recommendation: Re-add the one-line comment from the Modbus SubscribeCommand so
the S7 copy explains why the event handler writes via console.Output synchronously.
Resolution: Resolved 2026-05-23 — re-added the explanatory comment above the OnDataChange handler in the S7 SubscribeCommand, mirroring the Modbus copy: it explains the use of the CliFx IConsole.Output abstraction (rather than System.Console) and notes that the handler runs synchronously because it's raised from a driver background thread. Added SubscribeCommandConsoleHandlerCommentTests to guard the rationale against future copy-paste regressions.
Re-review 2026-06-19 (commit 111d6983)
Checklist coverage
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.S7.Cli-009, Driver.S7.Cli-010 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.S7.Cli-011 |
| 4 | Error handling & resilience | Driver.S7.Cli-008, Driver.S7.Cli-009 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | Driver.S7.Cli-012 |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | No issues found |
| 10 | Documentation & comments | No issues found |
Driver.S7.Cli-008
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/S7CommandBase.cs |
| Status | Resolved |
Description: S7CommandBase had no ValidateEndpoint() method, so an operator passing --port 0, --port 65536, or --timeout-ms -1 got an opaque socket or argument exception thrown deep inside the S7.Net library stack rather than a clean CommandException. The analogous ModbusCommandBase.ValidateEndpoint() already had this guard.
Recommendation: Add a ValidateEndpoint() method to S7CommandBase that validates Port (1..65535) and TimeoutMs (strictly positive) before the driver is created, and call it at the top of each command's ExecuteAsync. Add tests covering invalid port values, non-positive timeout, boundary port values (1 and 65535), and the happy path.
Resolution: Resolved 2026-06-19 — added ValidateEndpoint() to S7CommandBase (port 1..65535, TimeoutMs strictly positive, both throw CommandException with the flag name); called it in ProbeCommand, ReadCommand, WriteCommand, and SubscribeCommand; added S7EndpointValidationTests (8 cases).
Driver.S7.Cli-009
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs:61, Commands/ReadCommand.cs, Commands/WriteCommand.cs |
| Status | Resolved |
Description: Two related cancellation-handling gaps: (1) ProbeCommand caught OperationCanceledException without the when (ct.IsCancellationRequested) filter — a driver-internal timeout raised with a different CancellationToken would be caught and re-thrown from the wrong handler branch, masking the real error. (2) ReadCommand and WriteCommand had no OperationCanceledException handler at all — when the operator presses Ctrl+C during a connect or read, CliFx handles the exception silently, but the Modbus/AbCip CLI pattern of printing "Cancelled." and letting the caller see a clean one-line acknowledgement was missing.
Recommendation: (1) Add when (ct.IsCancellationRequested) to the ProbeCommand catch. (2) Wrap ReadCommand and WriteCommand's driver calls in a try/catch (OperationCanceledException) when (ct.IsCancellationRequested) that prints "Cancelled." and returns, matching the Modbus pattern. Add source-level tests to guard the patterns.
Resolution: Resolved 2026-06-19 — added when (ct.IsCancellationRequested) filter to ProbeCommand; wrapped ReadCommand and WriteCommand driver calls in try/catch (OperationCanceledException) when (ct.IsCancellationRequested) printing "Cancelled."; added CancellationHandlingTests (3 cases).
Driver.S7.Cli-010
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs |
| Status | Resolved |
Description: SubscribeCommand.ExecuteAsync did not call FlushLogging() in its finally block. DriverCommandBase.ConfigureLogging() documents that FlushLogging() must be called in a finally to prevent buffered Serilog output from being discarded on process exit. For the long-running subscribe command (which can run for minutes before Ctrl+C), reconnect warnings and other log lines emitted just before termination would be silently lost. The AbCip CLI subscribe command already had this call.
Recommendation: Add FlushLogging() as the last statement in SubscribeCommand's outer finally block. Guard with a source-level test to prevent regression.
Resolution: Resolved 2026-06-19 — added FlushLogging() as the final statement in SubscribeCommand's finally block; added FlushLoggingConventionTests (1 case).
Driver.S7.Cli-011
| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:61-67 |
| Status | Resolved |
Description: The OnDataChange handler in SubscribeCommand called console.Output.WriteLine without serialisation. OnDataChange is raised on the PollGroupEngine background timer thread; if two poll ticks complete close together (e.g. with a very short --interval-ms), concurrent handler invocations can interleave partial lines on the output. The Modbus CLI subscribe command guards against this with a lock(writeLock) wrapper; the S7 command was a copy-paste that dropped the lock.
Recommendation: Add a var writeLock = new object(); beside the driver declaration and wrap the console.Output.WriteLine call in lock (writeLock). Guard with a source-level test.
Resolution: Resolved 2026-06-19 — added var writeLock = new object() and wrapped console.Output.WriteLine in lock (writeLock) inside SubscribeCommand.OnDataChange; added SubscribeCommandWriteLockTests (1 case).
Driver.S7.Cli-012
| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/ProbeCommand.cs |
| Status | Deferred |
Description: The Modbus CLI ProbeCommand was enhanced (Driver.Modbus.Cli-006) with a ComputeVerdict method that derives a single OK / DEGRADED / FAIL headline from both the driver state and the probe-read snapshot StatusCode. This prevents the contradictory output Health: Healthy when the probe snapshot carries a Bad quality code. The S7 ProbeCommand has the same contradiction risk — it prints Health: {health.State} without cross-checking the snapshot quality.
Recommendation: Port the ComputeVerdict logic from the Modbus CLI to the S7 CLI: derive a verdict from health.State + snapshot[0].StatusCode and print it as the headline Verdict: line, keeping Health: for the raw driver state.
Resolution: Deferred — enhancement rather than bug; requires mirroring the Modbus CLI's ComputeVerdict pattern which is a design change not a correctness fix. No test regression risk from deferring.