# 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 | 4 |

## 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 | Open |

**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:** _(open)_

### Driver.S7.Cli-005

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` |
| Status | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_

### 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 | Open |

**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:** _(open)_