67ef6c4ebc
- Driver.S7.Cli-004: 'await using var driver' is the sole driver disposal path; dropped the redundant explicit await ShutdownAsync from each command's finally. - Driver.S7.Cli-005: deleted the stale empty tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ directory (the real test project lives under tests/Drivers/Cli/). - Driver.S7.Cli-006: S7CommandBaseBuildOptionsTests cover the probe toggle, timeout mapping, host/port/CPU/rack/slot wiring, and tag list passthrough. - Driver.S7.Cli-007: re-added the SubscribeCommand handler comment explaining the CliFx IConsole.Output usage and that the poll-thread raises events. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
12 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-05-22 |
| Commit reviewed | 76d35d1 |
| 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.