docs(code-reviews): regenerate index — 29 Low findings resolved

Batch 5 cleared Open findings in Driver.AbCip.Cli, Driver.AbLegacy.Cli, Driver.S7.Cli, Driver.TwinCAT.Cli, and Driver.Modbus.Cli. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(driver-modbus-cli): resolve Low code-review findings (Driver.Modbus.Cli-003,004,005,006,007,008)
2026-05-23 08:35:12 -04:00 · 2026-05-23 08:35:05 -04:00 · 2026-05-23 08:34:57 -04:00 · 2026-05-23 08:34:48 -04:00 · 2026-05-23 08:34:32 -04:00 · 2026-05-23 08:34:25 -04:00
48 changed files with 2069 additions and 189 deletions
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 6 |
+| Open findings | 0 |

 ## Checklist coverage

@@ -96,7 +96,7 @@ into `AbCipCommandBase`.
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `OnDataChange` handler writes change lines to `console.Output`
 (a `TextWriter`) from the driver's poll-engine callback thread, while the command's
@@ -112,7 +112,12 @@ during the watch loop widens it.
 writes during the subscription with a shared lock so poll-thread and main-thread
 output cannot interleave.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — moved the "Subscribed to ... Ctrl+C to stop."
+banner write to BEFORE `driver.OnDataChange +=` (and therefore before
+`SubscribeAsync`). With the handler not yet attached when the banner runs, the
+poll thread cannot fire change events into `console.Output` concurrently with the
+main-thread banner write. After `+=` the only writer to `console.Output` is the
+poll-thread handler, so no interleaving is possible.

 ### Driver.AbCip.Cli-004

@@ -121,7 +126,7 @@ output cannot interleave.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `--interval-ms` (`IntervalMs`) is taken verbatim and passed as
 `TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A
@@ -135,7 +140,16 @@ downstream component to sanitise operator input is fragile. `--timeout-ms` on
 `ExecuteAsync` / in `AbCipCommandBase`, throwing a `CommandException` with the
 accepted range when out of bounds.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added `SubscribeCommand.ValidateInterval(int)`
+(throws `CommandException` for `IntervalMs <= 0`) called at the top of
+`SubscribeCommand.ExecuteAsync`; moved `TimeoutMs > 0` validation into the
+`AbCipCommandBase.Timeout` getter (throws `CommandException` for non-positive
+`TimeoutMs`) and added a `_ = Timeout` touch in `SubscribeCommand.ExecuteAsync` to
+fire that guard before the driver opens. Other commands trip the same guard
+naturally via `BuildOptions(...).Timeout`. Regression tests
+`AbCipCommandBaseTests.Timeout_get_throws_CommandException_when_TimeoutMs_is_non_positive`
+and `SubscribeCommandIntervalTests.ValidateInterval_rejects_non_positive` cover
+both paths.

 ### Driver.AbCip.Cli-005

@@ -144,7 +158,7 @@ accepted range when out of bounds.
 | Severity | Low |
 | Category | Performance & resource management |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `ConfigureLogging` assigns a freshly created Serilog logger to the
 process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived
@@ -159,7 +173,12 @@ module's review.)
 `AppDomain.ProcessExit` or a `finally` in the command), or have the CLI use a
 disposable logger scoped to `ExecuteAsync`.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `Driver.Cli.Common` already exposes
+`DriverCommandBase.FlushLogging()` (a `Log.CloseAndFlush()` wrapper); the AB CIP
+CLI was not calling it. Added `FlushLogging()` in the `finally` block of all four
+commands (`ProbeCommand`, `ReadCommand`, `WriteCommand`, `SubscribeCommand`) so
+buffered Serilog output is flushed before the process exits, including the
+Ctrl+C-driven `subscribe` path. No edits to `Driver.Cli.Common` were needed.

 ### Driver.AbCip.Cli-006

@@ -168,7 +187,7 @@ disposable logger scoped to `ExecuteAsync`.
 | Severity | Low |
 | Category | Design-document adherence |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout`
 property with a getter derived from `TimeoutMs` and an empty `init` body
@@ -183,9 +202,13 @@ bug.
 **Recommendation:** Either drop the `init` accessor entirely (make the override a
 get-only expression-bodied property) or have the empty `init` throw
 `NotSupportedException` to make the "driven by TimeoutMs" contract explicit and
-fail-fast.
+fail-fast. (Drop is not viable because the abstract base declares `{ get; init; }`
+and an override must provide both accessors.)

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — the `init` accessor now throws
+`NotSupportedException` with a message pointing the caller at `TimeoutMs`. A new
+test `AbCipCommandBaseTests.Timeout_setter_is_inert_and_does_not_silently_swallow_assignments`
+asserts that an object-initializer assignment to `Timeout` fails fast.

 ### Driver.AbCip.Cli-007

@@ -194,7 +217,7 @@ fail-fast.
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The only test file covers `WriteCommand.ParseValue` and
 `ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for
@@ -213,7 +236,12 @@ driver CLIs.
 (`HostAddress`, `PlcFamily`, `DeviceName`), the supplied tag list, and the `Timeout`
 derived from `TimeoutMs`.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added
+`tests/.../AbCipCommandBaseTests.cs` covering `BuildOptions` (probe disabled,
+controller-browse disabled, alarm-projection disabled, single device with
+`HostAddress` / `PlcFamily` / `cli-{Family}` device name, tag list passed verbatim,
+`Timeout` derived from `TimeoutMs`) and `DriverInstanceId` (`abcip-cli-{Gateway}`),
+plus the `RejectStructure` guard (throws for `Structure`, no-op for atomic types).

 ### Driver.AbCip.Cli-008

@@ -222,7 +250,7 @@ derived from `TimeoutMs`.
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `docs/Driver.AbCip.Cli.md:8-9` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `docs/Driver.AbCip.Cli.md` opens with "Second of four driver
 test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four"
@@ -235,4 +263,6 @@ doc's "four" and the truncated chain are both stale.
 and complete the chain (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS), or
 drop the explicit count and link `docs/DriverClis.md` as the authoritative roster.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — rewrote the lead paragraph to "Second of six
+driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS)"
+and added a link to `docs/DriverClis.md` as the authoritative roster.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 6 |
+| Open findings | 0 |

 ## Checklist coverage

@@ -68,7 +68,7 @@ type (mirroring the existing `Bit` and unsupported-type branches). Either catch
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `--value` option help text states "booleans accept
 true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message
@@ -82,7 +82,10 @@ with both the code and the design doc.
 matching the wording used elsewhere (e.g. "booleans accept
 true/false, 1/0, on/off, yes/no").

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — updated `WriteCommand.Value` description to
+list the full alias set: "booleans accept true/false, 1/0, on/off, yes/no".
+Regression test `CommandMetadataTests.WriteCommand_value_help_lists_full_boolean_alias_set`
+asserts the description contains every alias group.

 ### Driver.AbLegacy.Cli-003

@@ -91,7 +94,7 @@ true/false, 1/0, on/off, yes/no").
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `Commands/SubscribeCommand.cs:47-53` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)`
 (the synchronous overload) directly from the `PollGroupEngine` poll thread. The
@@ -109,7 +112,10 @@ change events through a `Channel<string>` drained by a single consumer task, or
 guard the `WriteLine` with a lock. At minimum, document that the interleaving is
 accepted because output is human-facing and line-buffered.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` now serialises every
+console write through a shared `consoleGate` lock: the poll-thread
+`OnDataChange` callback and the command-thread "Subscribed to ..." line both take
+the lock before calling `WriteLine`. Comment in the source documents the intent.

 ### Driver.AbLegacy.Cli-004

@@ -118,7 +124,7 @@ accepted because output is human-facing and line-buffered.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` |
-| Status | Open |
+| Status | Resolved |

 **Description:** Every command does `await using var driver = new AbLegacyDriver(...)`
 *and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver`
@@ -135,7 +141,12 @@ cleanup on every exit path including exceptions.
 since the commands deliberately pass `CancellationToken.None` to shutdown so teardown
 is not cut short by a cancelled `ct`.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — replaced `await using var driver` with a
+plain `var driver` in all four commands (`ProbeCommand`, `ReadCommand`,
+`WriteCommand`, `SubscribeCommand`), keeping the explicit
+`finally { await driver.ShutdownAsync(CancellationToken.None) }` as the single
+teardown path. Comment in each command documents the intent so readers do not
+have to verify idempotency.

 ### Driver.AbLegacy.Cli-005

@@ -144,7 +155,7 @@ is not cut short by a cancelled `ct`.
 | Severity | Low |
 | Category | Design-document adherence |
 | Location | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The subscribe command interval option is `--interval-ms`
 (default 1000). `docs/Driver.AbLegacy.Cli.md` shows the subscribe example as
@@ -160,7 +171,13 @@ but the documented contract drifts between the two CLIs.
 `--interval-ms` description for parity with the AbCip CLI, and mention the
 `--interval-ms` long form + 1000 ms default in `docs/Driver.AbLegacy.Cli.md`.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — extended the `SubscribeCommand.IntervalMs`
+help text to match AbCip ("Publishing interval in milliseconds (default 1000).
+PollGroupEngine floors sub-250ms values.") and added a paragraph under the
+`subscribe` section in `docs/Driver.AbLegacy.Cli.md` naming the `-i` /
+`--interval-ms` long form, the 1000 ms default, and the 250 ms floor. Regression
+test `CommandMetadataTests.SubscribeCommand_interval_ms_help_notes_PollGroupEngine_floor`
+asserts the description mentions "250".

 ### Driver.AbLegacy.Cli-006

@@ -169,7 +186,7 @@ but the documented contract drifts between the two CLIs.
 | Severity | Low |
 | Category | Code organization & conventions |
 | Location | `Commands/ProbeCommand.cs:20-22` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `ProbeCommand` declares its `--type` option with no short alias,
 while `ReadCommand`, `WriteCommand`, and `SubscribeCommand` all declare `--type`
@@ -182,7 +199,13 @@ it silently rejected on `probe`.
 for consistency with the other three commands. (The AbCip CLI `ProbeCommand` has
 the same omission, so a cross-CLI sweep is worthwhile.)

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added the `'t'` short alias to
+`ProbeCommand.DataType`. Regression test
+`CommandMetadataTests.ProbeCommand_type_has_short_alias_t` (plus the parity test
+`Other_commands_keep_type_short_alias_t` for read/write/subscribe) asserts the
+short alias is present on every command. The same omission still exists in the
+AbCip CLI's `ProbeCommand` — flagged as a sibling sweep but out of scope for
+this module.

 ### Driver.AbLegacy.Cli-007

@@ -191,7 +214,7 @@ the same omission, so a cross-CLI sweep is worthwhile.)
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The only test file in the CLI test project covers
 `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that
@@ -210,4 +233,11 @@ Driver.AbLegacy.Cli-001. `BuildOptions` is reachable via `InternalsVisibleTo`
 tag passthrough) and an overflow-input test for `ParseValue` so the fix for
 Driver.AbLegacy.Cli-001 is locked in by a regression test.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added `BuildOptionsTests` (five tests:
+probe disabled, device shape from Gateway+PlcType, tag passthrough, timeout
+propagation, empty tag list) covering `AbLegacyCommandBase.BuildOptions` via a
+nested `TestCommand` subclass annotated with `[Command]` to satisfy the CliFx
+analyzer. The overflow path for `ParseValue` is already covered by
+`WriteCommandParseValueTests.ParseValue_out_of_range_throws_CommandException`
+(theory with `short.Parse` + `AnalogInt` overflow inputs), added when finding
+Driver.AbLegacy.Cli-001 was resolved.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 6 |
+| Open findings | 0 |

 ## Checklist coverage

@@ -87,7 +87,7 @@ message explaining coils carry a single bit.
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs:14-24` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `Port` (`int`) and `TimeoutMs` (`int`) accept any 32-bit value,
 including negatives and ports above 65535. `UnitId` is a `byte`, so it accepts
@@ -103,7 +103,13 @@ error. None of these are validated at parse time.
 message — consistent with how `WriteCommand` already rejects bad regions and
 boolean strings.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added `ModbusCommandBase.ValidateEndpoint()`
+that throws `CliFx.Exceptions.CommandException` for `--port` outside 1..65535,
+non-positive `--timeout-ms`, and `--unit-id` outside 1..247 (Modbus spec unicast
+range — rejects the broadcast 0 and reserved 248-255). Each of `ProbeCommand`,
+`ReadCommand`, `WriteCommand`, and `SubscribeCommand` now calls it at the top of
+`ExecuteAsync` after `ConfigureLogging()`. Regression tests live in
+`ModbusCommandBaseTests` (range + boundary cases for all three knobs).


 ### Driver.Modbus.Cli-004
@@ -113,7 +119,7 @@ boolean strings.
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:61-67` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `OnDataChange` handler is invoked from the driver's
 `PollGroupEngine` background thread and calls `console.Output.WriteLine`
@@ -127,7 +133,11 @@ any synchronization, so overlapping poll ticks could interleave partial lines.
 write failures so a transient console-write error cannot tear down the poll loop.
 A single `lock` around the write also removes the interleave risk.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — wrapped the `OnDataChange` handler in
+`try/catch (Exception)` that logs to Serilog at Warning and swallows the failure
+so a transient console-write error cannot fault the `PollGroupEngine` background
+loop. The console write is also serialized through a local `lock` object, removing
+the partial-line interleave risk when multiple poll ticks overlap.

 ### Driver.Modbus.Cli-005

@@ -136,7 +146,7 @@ A single `lock` around the write also removes the interleave risk.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:21-54`; `Commands/ReadCommand.cs:46-75`; `Commands/WriteCommand.cs:54-89` |
-| Status | Open |
+| Status | Resolved |

 **Description:** All three commands call `ConfigureLogging()` then
 `console.RegisterCancellationHandler()`, but if the operator presses Ctrl+C
@@ -152,7 +162,14 @@ commands do not catch it around their driver calls.
 the noisy trace on Ctrl+C-during-connect is acceptable. Consistency with
 `SubscribeCommand`'s handling is the cleaner choice.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added a
+`catch (OperationCanceledException) when (ct.IsCancellationRequested)` clause
+around the driver-call bodies in `ProbeCommand`, `ReadCommand`, and `WriteCommand`
+that prints `Cancelled.` and falls through to the existing `finally`-block
+shutdown. Matches `SubscribeCommand`'s existing handling so all four commands now
+exit quietly on Ctrl+C. Regression tests in `CommandCancellationTests` pre-cancel
+the CliFx `FakeInMemoryConsole` before calling `ExecuteAsync` and assert no
+exception escapes.

 ### Driver.Modbus.Cli-006

@@ -161,7 +178,7 @@ the noisy trace on Ctrl+C-during-connect is acceptable. Consistency with
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:35-53` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `probe` reports `Health: {health.State}` from `GetHealth()`.
 After a successful `InitializeAsync` the driver sets state to `Healthy`
@@ -179,7 +196,14 @@ snapshot's `StatusCode` (Good vs Bad) rather than — or in addition to — the
 `State`, or print a single combined verdict line so the two cannot contradict each
 other.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `ProbeCommand` now prints a single combined
+`Verdict:` line above the bare driver-state line; the headline is computed by a
+new `ProbeCommand.ComputeVerdict(DriverState, statusCode)` helper that combines
+the driver state with the probe snapshot's OPC UA quality class (top 2 bits of
+the StatusCode). Verdict is `FAIL` whenever the driver is not Healthy OR the
+snapshot is Bad, `DEGRADED` when the driver is Healthy but the snapshot is
+Uncertain, and `OK` only when both are Good. Regression tests in
+`ProbeCommandTests` pin the verdict-grid behaviour.

 ### Driver.Modbus.Cli-007

@@ -188,7 +212,7 @@ other.
 | Severity | Low |
 | Category | Design-document adherence |
 | Location | `docs/Driver.Modbus.Cli.md:124-156`; `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `docs/Driver.Modbus.Cli.md` devotes a whole "v2 addressing
 grammar" section to the industry-standard tag-address strings (`40001:F:CDAB`,
@@ -205,7 +229,14 @@ driver's address-string parser (and `--family` for the DL205/MELSEC native
 syntax), or scope the "v2 addressing grammar" section of the doc to note it
 applies to `DriverConfig` JSON and is not a CLI flag.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — chose the doc-scoping fix (an
+`--address-string` flag is a bigger feature that warrants its own design
+discussion). Added a "CLI scope" callout to the top of the
+`docs/Driver.Modbus.Cli.md` "v2 addressing grammar" section stating the CLI
+accepts only the structured `--region` + `--address` + `--type` triple and that
+the address-string grammar is a `DriverConfig` JSON feature. The pre-existing
+closing paragraph already said the same thing; the new callout makes it visible
+before the grammar examples instead of after them. Code surface left unchanged.

 ### Driver.Modbus.Cli-008

@@ -214,7 +245,7 @@ applies to `DriverConfig` JSON and is not a CLI flag.
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The test project covers only the two pure-function seams:
 `ReadCommand.SynthesiseTagName` and `WriteCommand.ParseValue`. There is no coverage
@@ -231,4 +262,19 @@ likely to regress and is currently untested. The validation gaps in findings
 setters and assert the produced `ModbusDriverOptions`). Once findings 002/003 are
 fixed, add tests for the new validation paths.

-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added four new test classes covering the
+previously untested branch logic:
+- `ModbusCommandBaseTests` — six tests over `BuildOptions` (probe disabled,
+  `TimeoutMs` → `Timeout` mapping, `AutoReconnect` tracking
+  `--disable-reconnect` in both directions, host/port/unit flow-through, tag
+  pass-through) plus the new `ValidateEndpoint` range-check tests for finding
+  003 (port 1..65535, timeout-ms > 0, unit-id 1..247).
+- `WriteCommandRegionValidationTests` — read-only region rejection
+  (DiscreteInputs / InputRegisters) and the Coils-non-Bool guard for finding
+  002.
+- `ProbeCommandTests` — the new `ComputeVerdict` helper for finding 006
+  (OK / DEGRADED / FAIL grid).
+- `CommandCancellationTests` — Ctrl+C-during-initialize for `ProbeCommand` /
+  `ReadCommand` / `WriteCommand` for finding 005.
+
+Total test count grew from 18 to 64; all pass.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 4 |
+| Open findings | 0 |

 ## Checklist coverage

@@ -120,7 +120,7 @@ unreachable device, not crash on it.
 | 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 |
+| 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.
@@ -136,7 +136,7 @@ not actually disposing.
 `subscribe` command `UnsubscribeAsync`. Alternatively drop `await using`
 and keep the explicit `finally`. Pick one disposal mechanism per command.

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

@@ -145,7 +145,7 @@ and keep the explicit `finally`. Pick one disposal mechanism per command.
 | Severity | Low |
 | Category | Code organization & conventions |
 | Location | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` |
-| Status | Open |
+| 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
@@ -158,7 +158,7 @@ grepping the tree for the S7 CLI test project.
 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)_
+**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

@@ -167,7 +167,7 @@ S7 CLI own orphaned test folder, so it belongs to this module cleanup.
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The only test file covers `WriteCommand.ParseValue` and
 `ReadCommand.SynthesiseTagName`. `S7CommandBase.BuildOptions` — which maps the
@@ -184,7 +184,7 @@ not be caught. `ParseValue` is also missing an explicit overflow-edge test (e.g.
 overflow case to the `ParseValue` numeric tests once Driver.S7.Cli-001 is resolved so
 the test asserts the wrapped `CommandException`.

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

@@ -193,7 +193,7 @@ the test asserts the wrapped `CommandException`.
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:45-51` |
-| Status | Open |
+| 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
@@ -206,4 +206,4 @@ 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)_
+**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.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 7 |
+| Open findings | 0 |

 ## Checklist coverage

@@ -40,7 +40,7 @@ a category produced nothing rather than leaving it blank.
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` |
-| Status | Open |
+| Status | Resolved |

 **Description:** Numeric command options are accepted without range validation. `--timeout-ms`
 feeds `Timeout => TimeSpan.FromMilliseconds(TimeoutMs)`; passing `--timeout-ms 0` or a negative
@@ -56,7 +56,16 @@ failure mode should be a readable up-front rejection.
 shared helper on `TwinCATCommandBase`) and throw `CliFx.Exceptions.CommandException` with a
 clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outside `1..65535`.

-**Resolution:** _(open)_
+**Resolution:** Added a virtual `Validate()` helper on `TwinCATCommandBase` that rejects
+`TimeoutMs <= 0` and `AmsPort` outside `1..65535` with a clean
+`CliFx.Exceptions.CommandException` carrying the offending value. `SubscribeCommand` overrides
+`Validate()` to add the `IntervalMs > 0` check. Each `ExecuteAsync` calls `Validate()` first
+so the CLI surfaces "bad argument" up-front instead of letting the driver fail with an opaque
+transport error. Covered by `TwinCATCommandBaseTests.Validate_rejects_zero_timeout`,
+`Validate_rejects_negative_timeout`, `Validate_rejects_out_of_range_ams_port` (theory, 4
+cases), `Validate_accepts_in_range_ams_port` (theory, 4 cases),
+`SubscribeCommand_validate_rejects_zero_interval`,
+`SubscribeCommand_validate_rejects_negative_interval`.

 ### Driver.TwinCAT.Cli-002

@@ -65,7 +74,7 @@ clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outsi
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `Commands/SubscribeCommand.cs:46-58` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)` synchronously.
 In native ADS-notification mode the event is raised from the `Beckhoff.TwinCAT.Ads`
@@ -83,7 +92,12 @@ serialised on one poll loop; the TwinCAT native path has no such serialisation.
 `TextWriter.Synchronized`. At minimum, gate it so the banner is written before the
 subscription is registered (it already is) and lock the per-event writes against each other.

-**Resolution:** _(open)_
+**Resolution:** Introduced a per-execution `writeLock` object inside
+`SubscribeCommand.ExecuteAsync`. Both the `OnDataChange` handler's `WriteLine` and the
+post-subscription "Subscribed to ..." banner take the lock, so notification-callback writes
+cannot interleave with the main-thread banner or with each other. Lock is local to the
+command so parallel process instances do not contend with one another (each owns its own
+console).

 ### Driver.TwinCAT.Cli-003

@@ -92,7 +106,7 @@ subscription is registered (it already is) and lock the per-event writes against
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `Commands/SubscribeCommand.cs:56-58` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The subscribe banner reports the mechanism purely from the `--poll-only` flag
 (`var mode = PollOnly ? "polling" : "ADS notification"`). The doc (`docs/Driver.TwinCAT.Cli.md`)
@@ -108,7 +122,15 @@ returned `ISubscriptionHandle.DiagnosticId`, which is `twincat-native-sub-*` for
 path vs the `PollGroupEngine` handle for poll mode) or soften the wording to "(requested:
 ADS notification)" so it does not over-claim.

-**Resolution:** _(open)_
+**Resolution:** Added internal static
+`SubscribeCommand.DescribeMechanism(ISubscriptionHandle)` that maps
+`DiagnosticId.StartsWith("twincat-native-sub-", Ordinal)` to `"ADS notification"` and
+anything else to `"polling"`. The banner now reads from the handle the driver actually
+returned, so the line cannot disagree with what the driver did even if a future fallback
+lands the subscription somewhere unexpected. Covered by
+`SubscribeCommandMechanismTests.DescribeMechanism_returns_ADS_notification_for_native_handle`
+(theory, 3 cases) and `DescribeMechanism_returns_polling_for_anything_else` (theory, 4 cases
+including an ordinal case-sensitivity guard).

 ### Driver.TwinCAT.Cli-004

@@ -117,7 +139,7 @@ ADS notification)" so it does not over-claim.
 | Severity | Low |
 | Category | Design-document adherence |
 | Location | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** `--poll-only` is declared on `TwinCATCommandBase`, so it is inherited by
 `browse`. `BrowseCommand` only ever calls `DiscoverAsync` — it never subscribes — so
@@ -131,7 +153,15 @@ disagree.
 flag) onto an intermediate base shared by only `probe`/`read`/`subscribe`, or override/hide it
 for `browse`. Alternatively document explicitly that the flag is a no-op for `browse`.

-**Resolution:** _(open)_
+**Resolution:** Introduced an intermediate `TwinCATTagCommandBase : TwinCATCommandBase` that
+hosts the `--poll-only` flag and the `BuildOptions(...)` helper. `ProbeCommand`,
+`ReadCommand`, `WriteCommand`, and `SubscribeCommand` inherit from this intermediate (they all
+build a tag-list `TwinCATDriverOptions`). `BrowseCommand` keeps inheriting from
+`TwinCATCommandBase` directly, so `--poll-only` no longer surfaces in `browse --help`. Browse
+sets `UseNativeNotifications = true` on its inline options (irrelevant either way for the
+discover-only path, but matches production wiring). Covered by
+`TwinCATCommandBaseTests.BrowseCommand_does_not_expose_poll_only_flag` and
+`ProbeCommand_still_exposes_poll_only_flag` (both reflect over the public property surface).

 ### Driver.TwinCAT.Cli-005

@@ -140,7 +170,7 @@ for `browse`. Alternatively document explicitly that the flag is a no-op for `br
 | Severity | Low |
 | Category | Code organization & conventions |
 | Location | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `--type` option is declared with the short alias `-t` on `read`, `write`,
 and `subscribe`, but `ProbeCommand` declares `[CommandOption("type", ...)]` with no short
@@ -151,7 +181,10 @@ take the same `TwinCATDataType` option.
 **Recommendation:** Add the `'t'` short alias to `ProbeCommand`'s `--type` option to match the
 other three commands.

-**Resolution:** _(open)_
+**Resolution:** Added the `'t'` short alias on `ProbeCommand.DataType`'s `[CommandOption]`,
+matching read/write/subscribe so muscle memory carries between the four verbs. Covered by
+`TwinCATCommandBaseTests.ProbeCommand_type_option_carries_short_alias_t` which asserts the
+`CommandOptionAttribute.ShortName` is `'t'`.

 ### Driver.TwinCAT.Cli-006

@@ -160,7 +193,7 @@ other three commands.
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The only test file covers `WriteCommand.ParseValue` and
 `ReadCommand.SynthesiseTagName`. Other deterministic, router-independent logic is untested:
@@ -177,7 +210,20 @@ module's scope but worth flagging to whoever owns the test tree.
 `BuildOptions` field wiring, and for the `CollectingAddressSpaceBuilder` prefix/max filtering
 and access-classification logic.

-**Resolution:** _(open)_
+**Resolution:** Added two new test classes. `TwinCATCommandBaseTests` covers the Gateway
+canonical form, round-trip through `TwinCATAmsAddress.TryParse`, DriverInstanceId
+composition, Timeout projection, BuildOptions field wiring (devices, tags, timeout, probe
+disabled, controller-browse disabled, UseNativeNotifications default true), and the PollOnly
+toggle flipping UseNativeNotifications. `BrowseCommandFilterTests` covers
+`CollectingAddressSpaceBuilder` (records variables in call order, treats `Folder` as a
+same-builder pass-through), `BrowseCommand.FilterByPrefix` (empty/null prefix passes
+everything, case-sensitive ordinal match), `BrowseCommand.PrintLimit` (max <= 0 = unbounded,
+caps when matched > max, no padding when matched < max), and `BrowseCommand.AccessTag`
+(ViewOnly -> RO, every other classification -> RW, theory over all 6 non-ViewOnly values).
+`BrowseCommand.CollectingAddressSpaceBuilder` made `internal` (was `private`) so the test
+project can construct it directly via the existing `InternalsVisibleTo` hook. Total tests
+for this assembly went from 27 to 69. The stale empty sibling test directory mention is
+left out of scope as noted.

 ### Driver.TwinCAT.Cli-007

@@ -186,7 +232,7 @@ and access-classification logic.
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `TwinCATCommandBase.cs:31-36` |
-| Status | Open |
+| Status | Resolved |

 **Description:** The `Timeout` override has an empty `init` accessor with the comment
 `/* driven by TimeoutMs */`. Because the base `DriverCommandBase.Timeout` is declared
@@ -199,4 +245,9 @@ gives no hint of the deliberate no-op. This is a maintainability/clarity nit, no
 computed projection of `--timeout-ms` and the `init` accessor is intentionally a no-op, so the
 design intent survives refactoring.

-**Resolution:** _(open)_
+**Resolution:** Replaced the `<inheritdoc/>` on `TwinCATCommandBase.Timeout` with an explicit
+`<summary>` documenting that `Timeout` is projected from `TimeoutMs`, that the `init`
+accessor required by the abstract base property is intentionally a no-op, and that adding a
+backing field would cause the two to drift on every refactor. The inner-block comment was
+tightened to point at the XML summary so the design intent survives whichever doc surface a
+future maintainer reads first.
@@ -23,9 +23,9 @@ Each module's `findings.md` is the source of truth; this file is generated from
 | [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
 | [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
 | [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 |
-| [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 |
+| [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 |
 | [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
-| [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 7 |
+| [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
 | [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 6 |
 | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
 | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 5 |
@@ -34,12 +34,12 @@ Each module's `findings.md` is the source of truth; this file is generated from
 | [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 10 |
 | [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
 | [Driver.Modbus.Addressing](Driver.Modbus.Addressing/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 9 |
-| [Driver.Modbus.Cli](Driver.Modbus.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 8 |
+| [Driver.Modbus.Cli](Driver.Modbus.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 |
 | [Driver.OpcUaClient](Driver.OpcUaClient/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 |
 | [Driver.S7](Driver.S7/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 |
-| [Driver.S7.Cli](Driver.S7.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 4 | 7 |
+| [Driver.S7.Cli](Driver.S7.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
 | [Driver.TwinCAT](Driver.TwinCAT/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 16 |
-| [Driver.TwinCAT.Cli](Driver.TwinCAT.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 7 | 7 |
+| [Driver.TwinCAT.Cli](Driver.TwinCAT.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
 | [Server](Server/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 |

 ## Pending findings
@@ -67,18 +67,6 @@ Findings with status `Open` or `In Progress`, ordered by severity.
 | Client.UI-009 | Low | Design-document adherence | `ViewModels/HistoryViewModel.cs:44-54` | `HistoryViewModel.AggregateTypes` exposes eight entries: `null` (Raw) plus Average, Minimum, Maximum, Count, Start, End, and `StandardDeviation`. `docs/Client.UI.md` ("Query Options" table) lists only "Raw (default), Average, Minimum, Maxi… |
 | Client.UI-010 | Low | Code organization & conventions | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` | `DateTimeRangePicker` declares `MinDateTimeProperty` / `MaxDateTimeProperty` styled properties with public CLR accessors, but neither is read anywhere in the control. `TryParseDateTime`, `OnStartLostFocus`, and `OnEndLostFocus` never clamp… |
 | Client.UI-011 | Low | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` | The certificate-store-path `TextBox` watermark reads `(default: AppData/LmxOpcUaClient/pki)`, referencing the legacy pre-task-#208 folder name. Per `CLAUDE.md` / `docs/Client.UI.md` the canonical path is now `{LocalAppData}/OtOpcUaClient/`… |
-| Driver.AbCip.Cli-003 | Low | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` | The `OnDataChange` handler writes change lines to `console.Output` (a `TextWriter`) from the driver's poll-engine callback thread, while the command's main flow concurrently writes the "Subscribed to ... Ctrl+C to stop." line on the CLI th… |
-| Driver.AbCip.Cli-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` | `--interval-ms` (`IntervalMs`) is taken verbatim and passed as `TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A zero or negative value produces a non-positive `TimeSpan`; the option description claims "Poll… |
-| Driver.AbCip.Cli-005 | Low | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` | `ConfigureLogging` assigns a freshly created Serilog logger to the process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived one-shot command (`probe`, `read`, `write`) the process exit flushes the console sink,… |
-| Driver.AbCip.Cli-006 | Low | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` | `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout` property with a getter derived from `TimeoutMs` and an empty `init` body (`init { /* driven by TimeoutMs */ }`). Because the override has no `[CommandOption]` attribute,… |
-| Driver.AbCip.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for `AbCipCommandBase.BuildOptions` (the flag-to-`AbCipDriverOptions` mapping that all four commands d… |
-| Driver.AbCip.Cli-008 | Low | Documentation & comments | `docs/Driver.AbCip.Cli.md:8-9` | `docs/Driver.AbCip.Cli.md` opens with "Second of four driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four" contradicts the chain that follows it (five names) and contradicts `docs/DriverClis.md`, whic… |
-| Driver.AbLegacy.Cli-002 | Low | Correctness & logic bugs | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` | The `--value` option help text states "booleans accept true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message also accept `on/off` and `yes/no`, and `DriverClis.md` documents the full `true/false/1/0/yes/no/on/off… |
-| Driver.AbLegacy.Cli-003 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:47-53` | The `OnDataChange` handler calls `console.Output.WriteLine(line)` (the synchronous overload) directly from the `PollGroupEngine` poll thread. The poll engine raises change events from a background timer/loop thread, so two ticks that fire… |
-| Driver.AbLegacy.Cli-004 | Low | Error handling & resilience | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` | Every command does `await using var driver = new AbLegacyDriver(...)` *and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver` `DisposeAsync` itself calls `ShutdownAsync`, so the driver is shut down twice on t… |
-| Driver.AbLegacy.Cli-005 | Low | Design-document adherence | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` | The subscribe command interval option is `--interval-ms` (default 1000). `docs/Driver.AbLegacy.Cli.md` shows the subscribe example as `otopcua-ablegacy-cli subscribe ... -i 500`, which works because of the short alias `'i'`, but the doc ne… |
-| Driver.AbLegacy.Cli-006 | Low | Code organization & conventions | `Commands/ProbeCommand.cs:20-22` | `ProbeCommand` declares its `--type` option with no short alias, while `ReadCommand`, `WriteCommand`, and `SubscribeCommand` all declare `--type` with the short alias `'t'`. `ProbeCommand` also gives `--address` the alias `'a'`, matching t… |
-| Driver.AbLegacy.Cli-007 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file in the CLI test project covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that are pure logic (testable without a device) are uncovered: (1) `AbLegacyCommandBase.BuildOptions` — that it… |
 | Driver.Cli.Common-004 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` | `FormatTable` calls `rows.Max(r => r.Tag.Length)` (and the same for the value and status columns) without guarding against empty input. When `tagNames` and `snapshots` are both empty (equal length, so the mismatch check at line 56 passes),… |
 | Driver.Cli.Common-006 | Low | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` | Two minor doc inaccuracies. (1) The comment at `SnapshotFormatter.cs:71` states the "source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed" — true only when every snapshot has a non-null `SourceTimestampUtc`. `For… |
 | Driver.FOCAS.Cli-001 | Low | Error handling & resilience | `Commands/WriteCommand.cs:58-68` | `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 inpu… |
@@ -91,23 +79,6 @@ Findings with status `Open` or `In Progress`, ordered by severity.
 | Driver.Historian.Wonderware.Client-006 | Low | Error handling & resilience | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` | `PipeChannel.InvokeAsync` retries exactly once on transport failure and otherwise propagates. The options expose `ReconnectInitialBackoff` and `ReconnectMaxBackoff` and `WonderwareHistorianClientOptions` documents them as exponential backo… |
 | Driver.Historian.Wonderware.Client-008 | Low | Security | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` | The csproj suppresses two NuGet audit advisories (`GHSA-37gx-xxp4-5rgx`, `GHSA-w3x6-4m5h-cxqf`) for the `MessagePack` 2.5.187 dependency with no inline comment recording why the suppression is safe, who reviewed it, or when it should be re… |
 | Driver.Historian.Wonderware.Client-010 | Low | Documentation & comments | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` | Two doc/behaviour mismatches. (1) The `Dispose()` XML comment asserts the underlying channel async cleanup is non-blocking so the `GetAwaiter()/GetResult()` bridge is safe. `PipeChannel.DisposeAsync` calls `ResetTransport()`, which invokes… |
-| Driver.Modbus.Cli-003 | Low | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs:14-24` | `Port` (`int`) and `TimeoutMs` (`int`) accept any 32-bit value, including negatives and ports above 65535. `UnitId` is a `byte`, so it accepts 0-255 even though the option description and `docs/Driver.Modbus.Cli.md` both say the valid rang… |
-| Driver.Modbus.Cli-004 | Low | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:61-67` | The `OnDataChange` handler is invoked from the driver's `PollGroupEngine` background thread and calls `console.Output.WriteLine` synchronously. An exception thrown inside this handler (e.g. an `IOException` on a redirected or closed stdout… |
-| Driver.Modbus.Cli-005 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:21-54`; `Commands/ReadCommand.cs:46-75`; `Commands/WriteCommand.cs:54-89` | All three commands call `ConfigureLogging()` then `console.RegisterCancellationHandler()`, but if the operator presses Ctrl+C before `InitializeAsync` completes, the resulting `OperationCancelledException` propagates out of `ExecuteAsync`… |
-| Driver.Modbus.Cli-006 | Low | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:35-53` | `probe` reports `Health: {health.State}` from `GetHealth()`. After a successful `InitializeAsync` the driver sets state to `Healthy` regardless of whether the subsequent probe register read returns Good or a Bad status code. `ReadAsync` do… |
-| Driver.Modbus.Cli-007 | Low | Design-document adherence | `docs/Driver.Modbus.Cli.md:124-156`; `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs` | `docs/Driver.Modbus.Cli.md` devotes a whole "v2 addressing grammar" section to the industry-standard tag-address strings (`40001:F:CDAB`, `HR1:I`, `C100`, `V2000:F:CDAB`, etc.) and says "set the per-tag `addressString` field instead of the… |
-| Driver.Modbus.Cli-008 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` | The test project covers only the two pure-function seams: `ReadCommand.SynthesiseTagName` and `WriteCommand.ParseValue`. There is no coverage for `WriteCommand`'s read-only-region rejection (`Region is not (Coils or HoldingRegisters)`), no… |
-| Driver.S7.Cli-004 | Low | Performance & resource management | `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` | 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 c… |
-| Driver.S7.Cli-005 | Low | Code organization & conventions | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` | 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 direct… |
-| Driver.S7.Cli-006 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` | 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 = fa… |
-| Driver.S7.Cli-007 | Low | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:45-51` | 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… |
-| Driver.TwinCAT.Cli-001 | Low | Correctness & logic bugs | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` | Numeric command options are accepted without range validation. `--timeout-ms` feeds `Timeout => TimeSpan.FromMilliseconds(TimeoutMs)`; passing `--timeout-ms 0` or a negative value yields `TimeSpan.Zero`/a negative `TimeSpan`, which is then… |
-| Driver.TwinCAT.Cli-002 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:46-58` | The `OnDataChange` handler calls `console.Output.WriteLine(line)` synchronously. In native ADS-notification mode the event is raised from the `Beckhoff.TwinCAT.Ads` notification callback thread (see `TwinCATDriver.SubscribeAsync`, which in… |
-| Driver.TwinCAT.Cli-003 | Low | Error handling & resilience | `Commands/SubscribeCommand.cs:56-58` | The subscribe banner reports the mechanism purely from the `--poll-only` flag (`var mode = PollOnly ? "polling" : "ADS notification"`). The doc (`docs/Driver.TwinCAT.Cli.md`) states the banner "announces which mechanism is in play". The CL… |
-| Driver.TwinCAT.Cli-004 | Low | Design-document adherence | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` | `--poll-only` is declared on `TwinCATCommandBase`, so it is inherited by `browse`. `BrowseCommand` only ever calls `DiscoverAsync` — it never subscribes — so `UseNativeNotifications = !PollOnly` has no observable effect on a browse run. Th… |
-| Driver.TwinCAT.Cli-005 | Low | Code organization & conventions | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` | The `--type` option is declared with the short alias `-t` on `read`, `write`, and `subscribe`, but `ProbeCommand` declares `[CommandOption("type", ...)]` with no short alias. An operator who has internalised `-t` from the other three verbs… |
-| Driver.TwinCAT.Cli-006 | Low | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` | The only test file covers `WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Other deterministic, router-independent logic is untested: `TwinCATCommandBase.Gateway` (the `ads://{netId}:{port}` string the driver's `TwinCATAmsAdd… |
-| Driver.TwinCAT.Cli-007 | Low | Documentation & comments | `TwinCATCommandBase.cs:31-36` | The `Timeout` override has an empty `init` accessor with the comment `/* driven by TimeoutMs */`. Because the base `DriverCommandBase.Timeout` is declared `abstract { get; init; }`, the override must supply an `init`, but here it silently… |

 ## Closed findings

@@ -343,9 +314,21 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | Driver.AbCip-012 | Low | Resolved | Performance & resource management | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` |
 | Driver.AbCip-013 | Low | Resolved | Design-document adherence | `AbCipDriverOptions.cs:70-73`, `PlcFamilies/AbCipPlcFamilyProfile.cs:13-19`, `LibplctagTagRuntime.cs:16-27` |
 | Driver.AbCip-015 | Low | Resolved | Documentation & comments | `AbCipDriver.cs:9-11`, `PlcTagHandle.cs:23-27,53-58`, `AbCipTemplateCache.cs:12-15`, `IAbCipTagEnumerator.cs:6-11`, `AbCipDriverOptions.cs:21` |
+| Driver.AbCip.Cli-003 | Low | Resolved | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` |
+| Driver.AbCip.Cli-004 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` |
+| Driver.AbCip.Cli-005 | Low | Resolved | Performance & resource management | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
+| Driver.AbCip.Cli-006 | Low | Resolved | Design-document adherence | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` |
+| Driver.AbCip.Cli-007 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` |
+| Driver.AbCip.Cli-008 | Low | Resolved | Documentation & comments | `docs/Driver.AbCip.Cli.md:8-9` |
 | Driver.AbLegacy-005 | Low | Resolved | OtOpcUa conventions | `AbLegacyDriver.cs` (whole file) |
 | Driver.AbLegacy-011 | Low | Resolved | Performance & resource management | `AbLegacyDriver.cs:440` |
 | Driver.AbLegacy-013 | Low | Resolved | Code organization & conventions | `AbLegacyDriver.cs:340-345`, `AbLegacyDriver.cs:238-264` |
+| Driver.AbLegacy.Cli-002 | Low | Resolved | Correctness & logic bugs | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` |
+| Driver.AbLegacy.Cli-003 | Low | Resolved | Concurrency & thread safety | `Commands/SubscribeCommand.cs:47-53` |
+| Driver.AbLegacy.Cli-004 | Low | Resolved | Error handling & resilience | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` |
+| Driver.AbLegacy.Cli-005 | Low | Resolved | Design-document adherence | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` |
+| Driver.AbLegacy.Cli-006 | Low | Resolved | Code organization & conventions | `Commands/ProbeCommand.cs:20-22` |
+| Driver.AbLegacy.Cli-007 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` |
 | Driver.FOCAS-007 | Low | Resolved | Error handling & resilience | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` |
 | Driver.FOCAS-008 | Low | Resolved | Performance & resource management | `FocasDriver.cs:201`, `FocasDriver.cs:253` |
 | Driver.FOCAS-009 | Low | Resolved | Design-document adherence | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |
@@ -372,6 +355,12 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | Driver.Modbus.Addressing-006 | Low | Resolved | Error handling & resilience | `ModbusAddressParser.cs:297-301` |
 | Driver.Modbus.Addressing-007 | Low | Resolved | Design-document adherence | `ModbusDataType.cs:91-95`, `docs/v2/dl205.md` section Strings |
 | Driver.Modbus.Addressing-009 | Low | Resolved | Documentation & comments | `ModbusModiconAddress.cs:55-64`, `ModbusModiconAddress.cs:104-110` |
+| Driver.Modbus.Cli-003 | Low | Resolved | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/ModbusCommandBase.cs:14-24` |
+| Driver.Modbus.Cli-004 | Low | Resolved | Concurrency & thread safety | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/SubscribeCommand.cs:61-67` |
+| Driver.Modbus.Cli-005 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:21-54`; `Commands/ReadCommand.cs:46-75`; `Commands/WriteCommand.cs:54-89` |
+| Driver.Modbus.Cli-006 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ProbeCommand.cs:35-53` |
+| Driver.Modbus.Cli-007 | Low | Resolved | Design-document adherence | `docs/Driver.Modbus.Cli.md:124-156`; `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli/Commands/ReadCommand.cs` |
+| Driver.Modbus.Cli-008 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests/` |
 | Driver.OpcUaClient-011 | Low | Resolved | Documentation & comments | `OpcUaClientDriver.cs:1007-1015` |
 | Driver.OpcUaClient-014 | Low | Resolved | Performance & resource management | `OpcUaClientDriver.cs:1138`, `:1314` |
 | Driver.S7-003 | Low | Resolved | Correctness & logic bugs | `S7Driver.cs:172`, `S7Driver.cs:255` |
@@ -379,11 +368,22 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | Driver.S7-009 | Low | Resolved | Error handling & resilience | `S7Driver.cs:392` |
 | Driver.S7-010 | Low | Resolved | Performance & resource management | `S7Driver.cs:504` |
 | Driver.S7-013 | Low | Resolved | Code organization & conventions | `S7DriverOptions.cs:90`, `S7Driver.cs:300` |
+| Driver.S7.Cli-004 | Low | Resolved | Performance & resource management | `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` |
+| Driver.S7.Cli-005 | Low | Resolved | Code organization & conventions | `tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/` |
+| Driver.S7.Cli-006 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/WriteCommandParseValueTests.cs` |
+| Driver.S7.Cli-007 | Low | Resolved | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli/Commands/SubscribeCommand.cs:45-51` |
 | Driver.TwinCAT-004 | Low | Resolved | Correctness & logic bugs | `TwinCATDataType.cs:24-27` |
 | Driver.TwinCAT-006 | Low | Resolved | OtOpcUa conventions | `TwinCATDriver.cs:406-411` |
 | Driver.TwinCAT-014 | Low | Resolved | Design-document adherence | `TwinCATDriverOptions.cs:41-43`, `TwinCATDriverOptions.cs:57-62`, `AdsTwinCATClient.cs:145` |
 | Driver.TwinCAT-015 | Low | Resolved | Code organization & conventions | `TwinCATDriver.cs:431-432` |
 | Driver.TwinCAT-016 | Low | Resolved | Testing coverage | `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/` |
+| Driver.TwinCAT.Cli-001 | Low | Resolved | Correctness & logic bugs | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` |
+| Driver.TwinCAT.Cli-002 | Low | Resolved | Concurrency & thread safety | `Commands/SubscribeCommand.cs:46-58` |
+| Driver.TwinCAT.Cli-003 | Low | Resolved | Error handling & resilience | `Commands/SubscribeCommand.cs:56-58` |
+| Driver.TwinCAT.Cli-004 | Low | Resolved | Design-document adherence | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` |
+| Driver.TwinCAT.Cli-005 | Low | Resolved | Code organization & conventions | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` |
+| Driver.TwinCAT.Cli-006 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` |
+| Driver.TwinCAT.Cli-007 | Low | Resolved | Documentation & comments | `TwinCATCommandBase.cs:31-36` |
 | Server-004 | Low | Resolved | OtOpcUa conventions | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs:187-200` |
 | Server-006 | Low | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:478-482, 1342-1348` |
 | Server-008 | Low | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:736` |
@@ -4,8 +4,9 @@ Ad-hoc probe / read / write / subscribe tool for ControlLogix / CompactLogix /
 Micro800 / GuardLogix PLCs, talking to the **same** `AbCipDriver` the OtOpcUa
 server uses (libplctag under the hood).

-Second of four driver test-client CLIs (Modbus → AB CIP → AB Legacy → S7 →
-TwinCAT). Shares `Driver.Cli.Common` with the others.
+Second of six driver test-client CLIs (Modbus → AB CIP → AB Legacy → S7 →
+TwinCAT → FOCAS). Shares `Driver.Cli.Common` with the others; see
+[DriverClis.md](DriverClis.md) for the authoritative roster.

 ## Build + run

@@ -95,6 +95,9 @@ PLC-managed — use with caution.
 otopcua-ablegacy-cli subscribe -g ab://192.168.1.20/1,0 -a N7:10 -t Int -i 500
 ```

+`-i` / `--interval-ms` is the publishing interval in milliseconds — default
+`1000`. `PollGroupEngine` floors sub-250 ms values, so `-i 100` runs at 250 ms.
+
 ## Known caveat — ab_server upstream gap

 The integration-fixture `ab_server` Docker container accepts TCP but its PCCC
@@ -122,6 +122,14 @@ gives plausible values is the correct one for that device.

 ## v2 addressing grammar

+> **CLI scope:** the `read` / `write` / `subscribe` commands accept only
+> the structured `--region` + `--address` + `--type` triple. The
+> address-string grammar below is a **`DriverConfig` JSON** feature
+> consumed by the driver itself; it is not reachable from this CLI's
+> flags. To experiment with it via the CLI, use the structured flags;
+> to deploy spreadsheets as-is, hand-author a `DriverConfig` and run
+> the server.
+
 The driver accepts the industry-standard tag-address grammar so you can
 paste tag spreadsheets from Wonderware / Kepware / Ignition without
 per-row manual translation. Full reference + grammar rules:
@@ -27,10 +27,28 @@ public abstract class AbCipCommandBase : DriverCommandBase
    public int TimeoutMs { get; init; } = 5000;

    /// <inheritdoc />
+    /// <remarks>
+    ///     The getter validates <see cref="TimeoutMs"/> (Driver.AbCip.Cli-004) — a zero or
+    ///     negative <c>--timeout-ms</c> would otherwise propagate as a non-positive
+    ///     <see cref="TimeSpan"/> into the driver. The <c>init</c> accessor is unreachable
+    ///     because CliFx binds <see cref="TimeoutMs"/> rather than <c>Timeout</c>; it throws
+    ///     <see cref="NotSupportedException"/> so an object-initializer assignment
+    ///     (<c>new ReadCommand { Timeout = ... }</c>) fails fast instead of being silently
+    ///     discarded (Driver.AbCip.Cli-006).
+    /// </remarks>
    public override TimeSpan Timeout
    {
-        get => TimeSpan.FromMilliseconds(TimeoutMs);
-        init { /* driven by TimeoutMs */ }
+        get
+        {
+            if (TimeoutMs <= 0)
+                throw new CliFx.Exceptions.CommandException(
+                    $"--timeout-ms must be > 0 (got {TimeoutMs}). " +
+                    "Pick a positive number of milliseconds for the per-operation timeout.");
+            return TimeSpan.FromMilliseconds(TimeoutMs);
+        }
+        init => throw new NotSupportedException(
+            $"{nameof(AbCipCommandBase)}.{nameof(Timeout)} is derived from {nameof(TimeoutMs)} " +
+            "and cannot be assigned directly. Set TimeoutMs instead.");
    }

    /// <summary>
@@ -54,6 +54,9 @@ public sealed class ProbeCommand : AbCipCommandBase
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
+            // Driver.AbCip.Cli-005 — flush Serilog before process exit so buffered log
+            // output emitted during driver shutdown is not lost.
+            FlushLogging();
        }
    }
 }
@@ -49,6 +49,9 @@ public sealed class ReadCommand : AbCipCommandBase
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
+            // Driver.AbCip.Cli-005 — flush Serilog before process exit so buffered log
+            // output emitted during driver shutdown is not lost.
+            FlushLogging();
        }
    }

@@ -31,6 +31,10 @@ public sealed class SubscribeCommand : AbCipCommandBase
    {
        ConfigureLogging();
        RejectStructure(DataType);
+        ValidateInterval(IntervalMs);
+        // Touch Timeout to surface the --timeout-ms guard (Driver.AbCip.Cli-004) before
+        // we open a driver — fast-fail with a clean CommandException on bad operator input.
+        _ = Timeout;
        var ct = console.RegisterCancellationHandler();

        var tagName = ReadCommand.SynthesiseTagName(TagPath, DataType);
@@ -48,6 +52,13 @@ public sealed class SubscribeCommand : AbCipCommandBase
        {
            await driver.InitializeAsync("{}", ct);

+            // Driver.AbCip.Cli-003 — emit the banner BEFORE wiring OnDataChange so the
+            // main-thread write cannot interleave with poll-thread change-event writes.
+            // TextWriter.WriteLine is not guaranteed thread-safe; once the handler is
+            // attached and SubscribeAsync starts, change events run on the poll thread.
+            await console.Output.WriteLineAsync(
+                $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop.");
+
            driver.OnDataChange += (_, e) =>
            {
                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
@@ -58,8 +69,6 @@ public sealed class SubscribeCommand : AbCipCommandBase

            handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);

-            await console.Output.WriteLineAsync(
-                $"Subscribed to {TagPath} @ {IntervalMs}ms. Ctrl+C to stop.");
            try
            {
                await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct);
@@ -77,6 +86,23 @@ public sealed class SubscribeCommand : AbCipCommandBase
                catch { /* teardown best-effort */ }
            }
            await driver.ShutdownAsync(CancellationToken.None);
+            // Driver.AbCip.Cli-005 — flush Serilog before process exit so buffered log
+            // lines emitted just before Ctrl+C are not lost on abrupt termination.
+            FlushLogging();
        }
    }
+
+    /// <summary>
+    ///     Guards <c>--interval-ms</c> against zero or negative values (Driver.AbCip.Cli-004).
+    ///     A non-positive interval would produce a non-positive <see cref="TimeSpan"/> into
+    ///     <c>SubscribeAsync</c>; the CLI should fail fast with an actionable error rather
+    ///     than relying on the downstream <c>PollGroupEngine</c> to clamp the value.
+    /// </summary>
+    internal static void ValidateInterval(int intervalMs)
+    {
+        if (intervalMs <= 0)
+            throw new CliFx.Exceptions.CommandException(
+                $"--interval-ms must be > 0 (got {intervalMs}). " +
+                "PollGroupEngine floors sub-250ms values, but accepts any positive integer.");
+    }
 }
@@ -60,6 +60,9 @@ public sealed class WriteCommand : AbCipCommandBase
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
+            // Driver.AbCip.Cli-005 — flush Serilog before process exit so buffered log
+            // output emitted during driver shutdown is not lost.
+            FlushLogging();
        }
    }

@@ -17,7 +17,7 @@ public sealed class ProbeCommand : AbLegacyCommandBase
        "the pre-populated register every SLC / MicroLogix / PLC-5 ships with.")]
    public string Address { get; init; } = "N7:0";

-    [CommandOption("type", Description =
+    [CommandOption("type", 't', Description =
        "PCCC data type of the probe address (default Int — matches N files).")]
    public AbLegacyDataType DataType { get; init; } = AbLegacyDataType.Int;

@@ -34,7 +34,10 @@ public sealed class ProbeCommand : AbLegacyCommandBase
            Writable: false);
        var options = BuildOptions([probeTag]);

-        await using var driver = new AbLegacyDriver(options, DriverInstanceId);
+        // Plain `var driver`: explicit ShutdownAsync(CancellationToken.None) in the
+        // finally is the deliberate teardown path; combining it with `await using`
+        // (which itself calls ShutdownAsync) would tear the driver down twice.
+        var driver = new AbLegacyDriver(options, DriverInstanceId);
        try
        {
            await driver.InitializeAsync("{}", ct);
@@ -36,7 +36,10 @@ public sealed class ReadCommand : AbLegacyCommandBase
            Writable: false);
        var options = BuildOptions([tag]);

-        await using var driver = new AbLegacyDriver(options, DriverInstanceId);
+        // Plain `var driver`: explicit ShutdownAsync(CancellationToken.None) in the
+        // finally is the deliberate teardown path; combining it with `await using`
+        // (which itself calls ShutdownAsync) would tear the driver down twice.
+        var driver = new AbLegacyDriver(options, DriverInstanceId);
        try
        {
            await driver.InitializeAsync("{}", ct);
@@ -21,7 +21,8 @@ public sealed class SubscribeCommand : AbLegacyCommandBase
    public AbLegacyDataType DataType { get; init; } = AbLegacyDataType.Int;

    [CommandOption("interval-ms", 'i', Description =
-        "Publishing interval in milliseconds (default 1000).")]
+        "Publishing interval in milliseconds (default 1000). PollGroupEngine floors " +
+        "sub-250ms values.")]
    public int IntervalMs { get; init; } = 1000;

    public override async ValueTask ExecuteAsync(IConsole console)
@@ -38,8 +39,17 @@ public sealed class SubscribeCommand : AbLegacyCommandBase
            Writable: false);
        var options = BuildOptions([tag]);

-        await using var driver = new AbLegacyDriver(options, DriverInstanceId);
+        // Plain `var driver` (no `await using`): driver.DisposeAsync internally calls
+        // ShutdownAsync, so combining `await using` with an explicit finally-shutdown
+        // would tear the driver down twice. The explicit teardown is preferred because
+        // it deliberately passes CancellationToken.None — `await using` would otherwise
+        // happen on a cancelled `ct` path which can cut teardown short.
+        var driver = new AbLegacyDriver(options, DriverInstanceId);
        ISubscriptionHandle? handle = null;
+        // Serialise console writes from the poll-thread OnDataChange callback against
+        // the command-thread "Subscribed to ..." line and against each other; the
+        // PollGroupEngine raises change events on a background timer/loop thread.
+        var consoleGate = new object();
        try
        {
            await driver.InitializeAsync("{}", ct);
@@ -49,13 +59,19 @@ public sealed class SubscribeCommand : AbLegacyCommandBase
                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
                    $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
                    $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
-                console.Output.WriteLine(line);
+                lock (consoleGate)
+                {
+                    console.Output.WriteLine(line);
+                }
            };

            handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);

-            await console.Output.WriteLineAsync(
-                $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
+            lock (consoleGate)
+            {
+                console.Output.WriteLine(
+                    $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
+            }
            try
            {
                await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct);
@@ -25,7 +25,7 @@ public sealed class WriteCommand : AbLegacyCommandBase
    public AbLegacyDataType DataType { get; init; } = AbLegacyDataType.Int;

    [CommandOption("value", 'v', Description =
-        "Value to write. Parsed per --type (booleans accept true/false/1/0).",
+        "Value to write. Parsed per --type (booleans accept true/false, 1/0, on/off, yes/no).",
        IsRequired = true)]
    public string Value { get; init; } = default!;

@@ -45,7 +45,10 @@ public sealed class WriteCommand : AbLegacyCommandBase

        var parsed = ParseValue(Value, DataType);

-        await using var driver = new AbLegacyDriver(options, DriverInstanceId);
+        // Plain `var driver`: explicit ShutdownAsync(CancellationToken.None) in the
+        // finally is the deliberate teardown path; combining it with `await using`
+        // (which itself calls ShutdownAsync) would tear the driver down twice.
+        var driver = new AbLegacyDriver(options, DriverInstanceId);
        try
        {
            await driver.InitializeAsync("{}", ct);
@@ -1,5 +1,6 @@
 using CliFx.Attributes;
 using CliFx.Infrastructure;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
 using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;

 namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands;
@@ -21,6 +22,7 @@ public sealed class ProbeCommand : ModbusCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
+        ValidateEndpoint();
        var ct = console.RegisterCancellationHandler();

        // Build with one probe tag + Probe.Enabled=false so InitializeAsync connects the
@@ -39,17 +41,59 @@ public sealed class ProbeCommand : ModbusCommandBase
            var snapshot = await driver.ReadAsync(["__probe"], ct);
            var health = driver.GetHealth();

+            // Driver.Modbus.Cli-006: derive the headline verdict from BOTH the driver state
+            // AND the probe-read StatusCode so the operator never sees the previous
+            // contradictory pair (`Health: Healthy` over a Bad snapshot line). The bare
+            // driver state is still printed below for diagnostics, but the verdict is what
+            // the operator scans first.
+            var verdict = ComputeVerdict(health.State, snapshot[0].StatusCode);
+
            await console.Output.WriteLineAsync($"Host:         {Host}:{Port} (unit {UnitId})");
-            await console.Output.WriteLineAsync($"Health:       {health.State}");
+            await console.Output.WriteLineAsync($"Verdict:      {verdict}");
+            await console.Output.WriteLineAsync($"Driver state: {health.State}");
            if (health.LastError is { } err)
                await console.Output.WriteLineAsync($"Last error:   {err}");
            await console.Output.WriteLineAsync();
            await console.Output.WriteLineAsync(
                SnapshotFormatter.Format($"HR[{ProbeAddress}]", snapshot[0]));
        }
+        catch (OperationCanceledException) when (ct.IsCancellationRequested)
+        {
+            // Driver.Modbus.Cli-005: Ctrl+C during InitializeAsync — exit quietly so CliFx
+            // does not render a full stack trace for a user-initiated cancellation.
+            await console.Output.WriteLineAsync("Cancelled.");
+        }
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
+
+    /// <summary>
+    ///     Driver.Modbus.Cli-006: combine the driver-side <see cref="DriverState"/> with the
+    ///     probe snapshot's OPC UA <c>StatusCode</c> into a single headline verdict. Order
+    ///     of precedence:
+    ///     <list type="number">
+    ///       <item><c>FAIL</c> — driver did not reach <see cref="DriverState.Healthy"/>
+    ///         (Faulted / Reconnecting / Unknown) OR the snapshot reports Bad quality.</item>
+    ///       <item><c>DEGRADED</c> — driver Healthy but the snapshot quality is Uncertain.</item>
+    ///       <item><c>OK</c> — driver Healthy and snapshot Good.</item>
+    ///     </list>
+    /// </summary>
+    public static string ComputeVerdict(DriverState state, uint statusCode)
+    {
+        // OPC UA StatusCode top 2 bits encode the quality class:
+        //   0x00xxxxxx → Good, 0x40xxxxxx → Uncertain, 0x80xxxxxx / 0xC0xxxxxx → Bad.
+        var qualityClass = statusCode & 0xC0000000u;
+        var snapshotGood = qualityClass == 0x00000000u;
+        var snapshotUncertain = qualityClass == 0x40000000u;
+
+        if (state != DriverState.Healthy || !snapshotGood && !snapshotUncertain)
+            return $"FAIL (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
+
+        if (snapshotUncertain)
+            return $"DEGRADED (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
+
+        return $"OK (driver={state}, probe={SnapshotFormatter.FormatStatus(statusCode)})";
+    }
 }
@@ -46,6 +46,7 @@ public sealed class ReadCommand : ModbusCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
+        ValidateEndpoint();
        var ct = console.RegisterCancellationHandler();

        var tagName = SynthesiseTagName(Region, Address, DataType);
@@ -68,6 +69,12 @@ public sealed class ReadCommand : ModbusCommandBase
            var snapshot = await driver.ReadAsync([tagName], ct);
            await console.Output.WriteLineAsync(SnapshotFormatter.Format(tagName, snapshot[0]));
        }
+        catch (OperationCanceledException) when (ct.IsCancellationRequested)
+        {
+            // Driver.Modbus.Cli-005: Ctrl+C during driver connect/read — exit quietly so
+            // CliFx does not render a full stack trace for a user-initiated cancellation.
+            await console.Output.WriteLineAsync("Cancelled.");
+        }
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
@@ -53,6 +53,7 @@ public sealed class SubscribeCommand : ModbusCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
+        ValidateEndpoint();
        var ct = console.RegisterCancellationHandler();

        var tagName = ReadCommand.SynthesiseTagName(Region, Address, DataType);
@@ -69,6 +70,9 @@ public sealed class SubscribeCommand : ModbusCommandBase
        var options = BuildOptions([tag]);

        await using var driver = new ModbusDriver(options, DriverInstanceId);
+        // Driver.Modbus.Cli-004: serialize console writes from the PollGroupEngine background
+        // thread so overlapping poll ticks can't interleave partial lines.
+        var writeLock = new object();
        ISubscriptionHandle? handle = null;
        try
        {
@@ -78,10 +82,26 @@ public sealed class SubscribeCommand : ModbusCommandBase
            // analyzer flags it + IConsole is the testable abstraction).
            driver.OnDataChange += (_, e) =>
            {
-                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
-                    $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
-                    $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
-                console.Output.WriteLine(line);
+                // Driver.Modbus.Cli-004: swallow + log write failures so a transient stdout
+                // error (closed pipe, IO exception on a redirected stream) cannot tear down
+                // the poll-engine background loop. Without this guard the unhandled
+                // exception would fault the long-running subscribe.
+                try
+                {
+                    var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
+                        $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
+                        $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
+                    lock (writeLock)
+                    {
+                        console.Output.WriteLine(line);
+                    }
+                }
+                catch (Exception ex)
+                {
+                    Serilog.Log.Logger.Warning(ex,
+                        "SubscribeCommand: console write failed for {Tag}; continuing poll loop.",
+                        e.FullReference);
+                }
            };

            handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);
@@ -54,6 +54,7 @@ public sealed class WriteCommand : ModbusCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
+        ValidateEndpoint();
        var ct = console.RegisterCancellationHandler();

        if (Region is not (ModbusRegion.Coils or ModbusRegion.HoldingRegisters))
@@ -92,6 +93,12 @@ public sealed class WriteCommand : ModbusCommandBase
            var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
            await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(tagName, results[0]));
        }
+        catch (OperationCanceledException) when (ct.IsCancellationRequested)
+        {
+            // Driver.Modbus.Cli-005: Ctrl+C during driver connect/write — exit quietly so
+            // CliFx does not render a full stack trace for a user-initiated cancellation.
+            await console.Output.WriteLineAsync("Cancelled.");
+        }
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
@@ -1,4 +1,5 @@
 using CliFx.Attributes;
+using CliFx.Exceptions;
 using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;

 namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli;
@@ -57,4 +58,33 @@ public abstract class ModbusCommandBase : DriverCommandBase
    ///     multiple endpoints in parallel can distinguish the logs.
    /// </summary>
    protected string DriverInstanceId => $"modbus-cli-{Host}:{Port}";
+
+    /// <summary>
+    ///     Driver.Modbus.Cli-003: validate the endpoint flags at parse time so the operator
+    ///     gets a clear CliFx error instead of an opaque socket / argument exception thrown
+    ///     deep inside the driver. Ranges:
+    ///     <list type="bullet">
+    ///       <item><c>--port</c>: 1..65535 (IANA TCP port space, excludes the
+    ///         "any" sentinel 0 and rejects negative / overflowed values).</item>
+    ///       <item><c>--timeout-ms</c>: strictly positive — a non-positive
+    ///         <see cref="TimeSpan"/> would propagate as an immediate-timeout into the
+    ///         driver and surface as a confusing TimeoutException.</item>
+    ///       <item><c>--unit-id</c>: 1..247 — the Modbus spec unicast unit-id range.
+    ///         0 is the broadcast address and not valid for read/write requests; 248-255
+    ///         are reserved. (Documented in <c>docs/Driver.Modbus.Cli.md</c>.)</item>
+    ///     </list>
+    /// </summary>
+    protected void ValidateEndpoint()
+    {
+        if (Port < 1 || Port > 65535)
+            throw new CommandException(
+                $"--port must be in 1..65535 (got {Port}).");
+        if (TimeoutMs <= 0)
+            throw new CommandException(
+                $"--timeout-ms must be strictly positive (got {TimeoutMs}).");
+        if (UnitId < 1 || UnitId > 247)
+            throw new CommandException(
+                $"--unit-id must be in 1..247 per the Modbus spec (got {UnitId}); " +
+                $"0 is the broadcast address, 248-255 are reserved.");
+    }
 }
@@ -33,6 +33,9 @@ public sealed class ProbeCommand : S7CommandBase
            Writable: false);
        var options = BuildOptions([probeTag]);

+        // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.DisposeAsync
+        // already invokes ShutdownAsync, so the previous explicit ShutdownAsync(CancellationToken.None)
+        // call in a finally block ran shutdown twice. The await-using on the next line is enough.
        await using var driver = new S7Driver(options, DriverInstanceId);
        // Driver.S7.Cli-003: wrap the entire probe sequence so that a refused/unreachable TCP
        // connect still prints the structured Host/CPU/Health lines instead of crashing with a
@@ -66,9 +69,5 @@ public sealed class ProbeCommand : S7CommandBase
            if (health.LastError is { } err)
                await console.Output.WriteLineAsync($"Last error:   {err}");
        }
-        finally
-        {
-            await driver.ShutdownAsync(CancellationToken.None);
-        }
    }
 }
@@ -45,17 +45,13 @@ public sealed class ReadCommand : S7CommandBase
            StringLength: StringLength);
        var options = BuildOptions([tag]);

+        // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.DisposeAsync
+        // already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None)
+        // in a finally block ran shutdown twice. The await-using on the next line is enough.
        await using var driver = new S7Driver(options, DriverInstanceId);
-        try
-        {
-            await driver.InitializeAsync("{}", ct);
-            var snapshot = await driver.ReadAsync([tagName], ct);
-            await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
-        }
-        finally
-        {
-            await driver.ShutdownAsync(CancellationToken.None);
-        }
+        await driver.InitializeAsync("{}", ct);
+        var snapshot = await driver.ReadAsync([tagName], ct);
+        await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
    }

    /// <summary>Tag-name key used internally. Address + type is already unique.</summary>
@@ -37,12 +37,20 @@ public sealed class SubscribeCommand : S7CommandBase
            Writable: false);
        var options = BuildOptions([tag]);

+        // Driver.S7.Cli-004: `await using` is the sole driver-disposal mechanism — S7Driver.DisposeAsync
+        // already invokes ShutdownAsync, so a redundant ShutdownAsync(CancellationToken.None) in finally
+        // ran shutdown twice. Only UnsubscribeAsync stays in the finally block — that's a subscription
+        // lifecycle concern that is not part of driver disposal.
        await using var driver = new S7Driver(options, DriverInstanceId);
        ISubscriptionHandle? handle = null;
        try
        {
            await driver.InitializeAsync("{}", ct);

+            // Driver.S7.Cli-007: route every data-change event to the CliFx console (not
+            // System.Console — the analyzer flags it + IConsole is the testable abstraction).
+            // The handler is synchronous because OnDataChange is raised from a driver
+            // background thread; the IConsole.Output writer is thread-safe for line writes.
            driver.OnDataChange += (_, e) =>
            {
                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
@@ -71,7 +79,6 @@ public sealed class SubscribeCommand : S7CommandBase
                try { await driver.UnsubscribeAsync(handle, CancellationToken.None); }
                catch { /* teardown best-effort */ }
            }
-            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
 }
@@ -52,17 +52,13 @@ public sealed class WriteCommand : S7CommandBase

        var parsed = ParseValue(Value, DataType);

+        // Driver.S7.Cli-004: `await using` is the sole disposal mechanism — S7Driver.DisposeAsync
+        // already invokes ShutdownAsync, so a redundant explicit ShutdownAsync(CancellationToken.None)
+        // in a finally block ran shutdown twice. The await-using on the next line is enough.
        await using var driver = new S7Driver(options, DriverInstanceId);
-        try
-        {
-            await driver.InitializeAsync("{}", ct);
-            var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
-            await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
-        }
-        finally
-        {
-            await driver.ShutdownAsync(CancellationToken.None);
-        }
+        await driver.InitializeAsync("{}", ct);
+        var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
+        await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
    }

    /// <summary>Parse <c>--value</c> per <see cref="S7DataType"/>, invariant culture throughout.</summary>
@@ -10,6 +10,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
 ///     when <c>EnableControllerBrowse = true</c> — structured UDTs / function-block instances
 ///     won't appear because the driver filters to the supported primitive surface.
 /// </summary>
+/// <remarks>
+///     Inherits from <see cref="TwinCATCommandBase"/> rather than
+///     <see cref="TwinCATTagCommandBase"/> so the <c>--poll-only</c> flag does NOT surface in
+///     <c>browse --help</c>: browse never subscribes, the flag would be a no-op, and the help
+///     text would mislead users (Driver.TwinCAT.Cli-004).
+/// </remarks>
 [Command("browse", Description = "Enumerate controller symbols via the driver's DiscoverAsync walk.")]
 public sealed class BrowseCommand : TwinCATCommandBase
 {
@@ -25,18 +31,21 @@ public sealed class BrowseCommand : TwinCATCommandBase

    public override async ValueTask ExecuteAsync(IConsole console)
    {
+        Validate();
        ConfigureLogging();
        var ct = console.RegisterCancellationHandler();

        // Browse-only — no declared tags. EnableControllerBrowse=true flips DiscoverAsync's
-        // symbol-walk on so every recognized primitive surfaces through the builder.
+        // symbol-walk on so every recognized primitive surfaces through the builder. Native
+        // ADS notifications are irrelevant here (DiscoverAsync never subscribes); leave the
+        // default on so the options record matches the production wiring.
        var options = new TwinCATDriverOptions
        {
            Devices = [new TwinCATDeviceOptions(Gateway, $"cli-{AmsNetId}:{AmsPort}")],
            Tags = [],
            Timeout = Timeout,
            Probe = new TwinCATProbeOptions { Enabled = false },
-            UseNativeNotifications = !PollOnly,
+            UseNativeNotifications = true,
            EnableControllerBrowse = true,
        };

@@ -52,10 +61,8 @@ public sealed class BrowseCommand : TwinCATCommandBase
            await driver.ShutdownAsync(CancellationToken.None);
        }

-        var matched = builder.Variables
-            .Where(v => string.IsNullOrEmpty(Prefix) || v.BrowseName.StartsWith(Prefix, StringComparison.Ordinal))
-            .ToList();
-        var printLimit = Max <= 0 ? matched.Count : Math.Min(Max, matched.Count);
+        var matched = FilterByPrefix(builder.Variables, Prefix);
+        var printLimit = PrintLimit(matched.Count, Max);

        await console.Output.WriteLineAsync($"AMS:          {AmsNetId}:{AmsPort}");
        await console.Output.WriteLineAsync(
@@ -64,8 +71,7 @@ public sealed class BrowseCommand : TwinCATCommandBase

        foreach (var v in matched.Take(printLimit))
        {
-            var access = v.Info.SecurityClass == SecurityClassification.ViewOnly ? "RO" : "RW";
-            await console.Output.WriteLineAsync($"  [{access}] {v.Info.DriverDataType,-8} {v.BrowseName}");
+            await console.Output.WriteLineAsync($"  [{AccessTag(v.Info)}] {v.Info.DriverDataType,-8} {v.BrowseName}");
        }

        if (matched.Count > printLimit)
@@ -73,7 +79,35 @@ public sealed class BrowseCommand : TwinCATCommandBase
                $"  … {matched.Count - printLimit} more — raise --max or tighten --prefix");
    }

-    private sealed class CollectingAddressSpaceBuilder : IAddressSpaceBuilder
+    /// <summary>
+    ///     Case-sensitive prefix filter. A null/empty prefix keeps everything; otherwise we
+    ///     keep symbols whose browse name starts with <paramref name="prefix"/> under
+    ///     <see cref="StringComparison.Ordinal"/> — TwinCAT identifiers are case-sensitive on
+    ///     the wire, so a relaxed match would be misleading.
+    /// </summary>
+    internal static List<(string BrowseName, DriverAttributeInfo Info)> FilterByPrefix(
+        IReadOnlyList<(string BrowseName, DriverAttributeInfo Info)> source, string? prefix)
+        => source
+            .Where(v => string.IsNullOrEmpty(prefix) || v.BrowseName.StartsWith(prefix, StringComparison.Ordinal))
+            .ToList();
+
+    /// <summary>
+    ///     Cap-to-max projection. <paramref name="max"/> &lt;= 0 means unbounded, otherwise the
+    ///     min of <paramref name="matchedCount"/> and <paramref name="max"/>.
+    /// </summary>
+    internal static int PrintLimit(int matchedCount, int max)
+        => max <= 0 ? matchedCount : Math.Min(max, matchedCount);
+
+    /// <summary>
+    ///     Coarse RO/RW label used in the browse output. <see cref="SecurityClassification.ViewOnly"/>
+    ///     is the only classification that is unconditionally read-only; everything else can be
+    ///     written from at least one ACL tier, so the CLI labels it RW. The real per-tier
+    ///     authorization is enforced server-side.
+    /// </summary>
+    internal static string AccessTag(DriverAttributeInfo info)
+        => info.SecurityClass == SecurityClassification.ViewOnly ? "RO" : "RW";
+
+    internal sealed class CollectingAddressSpaceBuilder : IAddressSpaceBuilder
    {
        public List<(string BrowseName, DriverAttributeInfo Info)> Variables { get; } = [];

@@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
 ///     server near the endpoint.
 /// </summary>
 [Command("probe", Description = "Verify the TwinCAT runtime is reachable and a sample symbol reads.")]
-public sealed class ProbeCommand : TwinCATCommandBase
+public sealed class ProbeCommand : TwinCATTagCommandBase
 {
    [CommandOption("symbol", 's', Description =
        "Symbol path to probe. System-global examples: " +
@@ -20,11 +20,14 @@ public sealed class ProbeCommand : TwinCATCommandBase
        IsRequired = true)]
    public string SymbolPath { get; init; } = default!;

-    [CommandOption("type", Description = "Data type (default DInt — TwinCAT DINT maps to int32).")]
+    [CommandOption("type", 't', Description =
+        "Bool / SInt / USInt / Int / UInt / DInt / UDInt / LInt / ULInt / Real / LReal / " +
+        "String / WString / Time / Date / DateTime / TimeOfDay (default DInt).")]
    public TwinCATDataType DataType { get; init; } = TwinCATDataType.DInt;

    public override async ValueTask ExecuteAsync(IConsole console)
    {
+        Validate();
        ConfigureLogging();
        var ct = console.RegisterCancellationHandler();

@@ -9,7 +9,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
 ///     member list into individual reads if you need them.
 /// </summary>
 [Command("read", Description = "Read a single TwinCAT symbol.")]
-public sealed class ReadCommand : TwinCATCommandBase
+public sealed class ReadCommand : TwinCATTagCommandBase
 {
    [CommandOption("symbol", 's', Description =
        "Symbol path. Program scope: 'MAIN.bStart'. Global: 'GVL.Counter'. " +
@@ -24,6 +24,7 @@ public sealed class ReadCommand : TwinCATCommandBase

    public override async ValueTask ExecuteAsync(IConsole console)
    {
+        Validate();
        ConfigureLogging();
        var ct = console.RegisterCancellationHandler();

@@ -1,4 +1,5 @@
 using CliFx.Attributes;
+using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
 using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;
@@ -10,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
 ///     pushes on its own cycle); pass <c>--poll-only</c> to fall through to PollGroupEngine.
 /// </summary>
 [Command("subscribe", Description = "Watch a TwinCAT symbol via ADS notification or poll, until Ctrl+C.")]
-public sealed class SubscribeCommand : TwinCATCommandBase
+public sealed class SubscribeCommand : TwinCATTagCommandBase
 {
    [CommandOption("symbol", 's', Description = "Symbol path — same format as `read`.", IsRequired = true)]
    public string SymbolPath { get; init; } = default!;
@@ -23,8 +24,17 @@ public sealed class SubscribeCommand : TwinCATCommandBase
    [CommandOption("interval-ms", 'i', Description = "Publishing interval ms (default 1000).")]
    public int IntervalMs { get; init; } = 1000;

+    protected override void Validate()
+    {
+        base.Validate();
+        if (IntervalMs <= 0)
+            throw new CommandException(
+                $"--interval-ms must be greater than 0 (got {IntervalMs}).");
+    }
+
    public override async ValueTask ExecuteAsync(IConsole console)
    {
+        Validate();
        ConfigureLogging();
        var ct = console.RegisterCancellationHandler();

@@ -43,19 +53,39 @@ public sealed class SubscribeCommand : TwinCATCommandBase
        {
            await driver.InitializeAsync("{}", ct);

+            // Native ADS notifications fire OnDataChange from the Beckhoff.TwinCAT.Ads
+            // notification callback thread — unlike the poll-mode path (which serialises on a
+            // single PollGroupEngine loop), the native callback can interleave with the banner
+            // write below and with subsequent change events if the PLC pushes faster than a
+            // single console write completes. A TextWriter is not guaranteed thread-safe, so
+            // we serialise every write through a lock to keep output clean for
+            // screen-recorded bug-report timelines (Driver.TwinCAT.Cli-002).
+            var writeLock = new object();
+
            driver.OnDataChange += (_, e) =>
            {
                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
                    $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
                    $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
-                console.Output.WriteLine(line);
+                lock (writeLock)
+                {
+                    console.Output.WriteLine(line);
+                }
            };

            handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);

-            var mode = PollOnly ? "polling" : "ADS notification";
-            await console.Output.WriteLineAsync(
-                $"Subscribed to {SymbolPath} @ {IntervalMs}ms ({mode}). Ctrl+C to stop.");
+            // Driver.TwinCAT.Cli-003: derive the banner mechanism from the actual subscription
+            // handle the driver returned, not from --poll-only. The native ADS path tags its
+            // handle with a "twincat-native-sub-*" DiagnosticId; anything else means we landed
+            // on the shared PollGroupEngine. That way the line cannot disagree with what the
+            // driver actually did (e.g. a future fallback inside SubscribeAsync).
+            var mode = DescribeMechanism(handle);
+            lock (writeLock)
+            {
+                console.Output.WriteLine(
+                    $"Subscribed to {SymbolPath} @ {IntervalMs}ms ({mode}). Ctrl+C to stop.");
+            }
            try
            {
                await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct);
@@ -75,4 +105,16 @@ public sealed class SubscribeCommand : TwinCATCommandBase
            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
+
+    /// <summary>
+    ///     Maps the returned subscription handle's <see cref="ISubscriptionHandle.DiagnosticId"/>
+    ///     to the banner label. The TwinCAT driver tags native ADS subscriptions with
+    ///     <c>twincat-native-sub-*</c> and the shared <c>PollGroupEngine</c> handle uses a
+    ///     different format — anything else means we landed on the poll loop. Internal so the
+    ///     test assembly can cover the mapping without spinning a real driver.
+    /// </summary>
+    internal static string DescribeMechanism(ISubscriptionHandle handle) =>
+        handle.DiagnosticId.StartsWith("twincat-native-sub-", StringComparison.Ordinal)
+            ? "ADS notification"
+            : "polling";
 }
@@ -11,7 +11,7 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
 ///     JSON for those.
 /// </summary>
 [Command("write", Description = "Write a single TwinCAT symbol.")]
-public sealed class WriteCommand : TwinCATCommandBase
+public sealed class WriteCommand : TwinCATTagCommandBase
 {
    [CommandOption("symbol", 's', Description =
        "Symbol path — same format as `read`.", IsRequired = true)]
@@ -29,6 +29,7 @@ public sealed class WriteCommand : TwinCATCommandBase

    public override async ValueTask ExecuteAsync(IConsole console)
    {
+        Validate();
        ConfigureLogging();
        var ct = console.RegisterCancellationHandler();

@@ -1,13 +1,15 @@
 using CliFx.Attributes;
+using CliFx.Exceptions;
 using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;

 namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli;

 /// <summary>
 ///     Base for every TwinCAT CLI command. Carries the AMS target options
-///     (<c>--ams-net-id</c> + <c>--ams-port</c>) + the notification-mode toggle that the
-///     driver itself takes. Exposes <see cref="BuildOptions"/> so each command can build a
-///     single-device / single-tag <see cref="TwinCATDriverOptions"/> from flag input.
+///     (<c>--ams-net-id</c> + <c>--ams-port</c>) + the per-call timeout. Commands that build
+///     a single-device / single-tag <see cref="TwinCATDriverOptions"/> from flag input inherit
+///     from <see cref="TwinCATTagCommandBase"/> instead — that intermediate adds the
+///     <c>--poll-only</c> flag and the <c>BuildOptions</c> helper.
 /// </summary>
 public abstract class TwinCATCommandBase : DriverCommandBase
 {
@@ -23,16 +25,19 @@ public abstract class TwinCATCommandBase : DriverCommandBase
    [CommandOption("timeout-ms", Description = "Per-operation timeout in ms (default 5000).")]
    public int TimeoutMs { get; init; } = 5000;

-    [CommandOption("poll-only", Description =
-        "Disable native ADS notifications and fall through to the shared PollGroupEngine " +
-        "(same as setting UseNativeNotifications=false in a real driver config).")]
-    public bool PollOnly { get; init; }
-
-    /// <inheritdoc />
+    /// <summary>
+    ///     The per-operation timeout, projected from <see cref="TimeoutMs"/>. The CliFx
+    ///     <c>init</c> accessor required by the abstract base property is intentionally a
+    ///     no-op: <see cref="TimeoutMs"/> is the only source of truth, so any value an
+    ///     `init` initialiser supplies to <see cref="Timeout"/> directly is silently
+    ///     dropped. Do NOT add a backing field "fixing" the empty body — it would diverge
+    ///     from <see cref="TimeoutMs"/> and the two would drift on every refactor
+    ///     (Driver.TwinCAT.Cli-007).
+    /// </summary>
    public override TimeSpan Timeout
    {
        get => TimeSpan.FromMilliseconds(TimeoutMs);
-        init { /* driven by TimeoutMs */ }
+        init { /* see XML summary — driven by TimeoutMs */ }
    }

    /// <summary>
@@ -41,22 +46,29 @@ public abstract class TwinCATCommandBase : DriverCommandBase
    /// </summary>
    protected string Gateway => $"ads://{AmsNetId}:{AmsPort}";

-    /// <summary>
-    ///     Build a <see cref="TwinCATDriverOptions"/> with the AMS target this base collected +
-    ///     the tag list a subclass supplies. Probe disabled, controller-browse disabled,
-    ///     native notifications toggled by <see cref="PollOnly"/>.
-    /// </summary>
-    protected TwinCATDriverOptions BuildOptions(IReadOnlyList<TwinCATTagDefinition> tags) => new()
-    {
-        Devices = [new TwinCATDeviceOptions(
-            HostAddress: Gateway,
-            DeviceName: $"cli-{AmsNetId}:{AmsPort}")],
-        Tags = tags,
-        Timeout = Timeout,
-        Probe = new TwinCATProbeOptions { Enabled = false },
-        UseNativeNotifications = !PollOnly,
-        EnableControllerBrowse = false,
-    };
-
    protected string DriverInstanceId => $"twincat-cli-{AmsNetId}:{AmsPort}";
+
+    /// <summary>
+    ///     Validates the numeric options every TwinCAT CLI command shares (timeout + AMS port).
+    ///     Subclasses override and call <c>base.Validate()</c> first to add their own range
+    ///     checks. Throwing here surfaces a clean CliFx one-line error before the driver gets
+    ///     a chance to fail with an opaque transport error (Driver.TwinCAT.Cli-001).
+    /// </summary>
+    protected virtual void Validate()
+    {
+        if (TimeoutMs <= 0)
+            throw new CommandException(
+                $"--timeout-ms must be greater than 0 (got {TimeoutMs}).");
+        if (AmsPort is <= 0 or > 65535)
+            throw new CommandException(
+                $"--ams-port must be in the range 1..65535 (got {AmsPort}).");
+    }
+
+    // ---- Test hooks ----
+    // Protected members are exposed to the test assembly through these internal accessors so the
+    // test project can cover Gateway / DriverInstanceId composition + range validation without
+    // needing reflection on every assertion (Driver.TwinCAT.Cli-006).
+    internal string GatewayForTest => Gateway;
+    internal string DriverInstanceIdForTest => DriverInstanceId;
+    internal void ValidateForTest() => Validate();
 }
@@ -0,0 +1,40 @@
+using CliFx.Attributes;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli;
+
+/// <summary>
+///     Intermediate base for the four TwinCAT CLI commands that build a single-device /
+///     single-tag <see cref="TwinCATDriverOptions"/> — <c>probe</c>, <c>read</c>, <c>write</c>,
+///     <c>subscribe</c>. Adds the <c>--poll-only</c> flag (relevant only when the driver is
+///     about to register native ADS notifications, which is why it does NOT live on the
+///     <c>browse</c> command — Driver.TwinCAT.Cli-004) and the <c>BuildOptions</c> helper that
+///     assembles the driver-side options record.
+/// </summary>
+public abstract class TwinCATTagCommandBase : TwinCATCommandBase
+{
+    [CommandOption("poll-only", Description =
+        "Disable native ADS notifications and fall through to the shared PollGroupEngine " +
+        "(same as setting UseNativeNotifications=false in a real driver config).")]
+    public bool PollOnly { get; init; }
+
+    /// <summary>
+    ///     Build a <see cref="TwinCATDriverOptions"/> with the AMS target this base collected +
+    ///     the tag list a subclass supplies. Probe disabled, controller-browse disabled,
+    ///     native notifications toggled by <see cref="PollOnly"/>.
+    /// </summary>
+    protected TwinCATDriverOptions BuildOptions(IReadOnlyList<TwinCATTagDefinition> tags) => new()
+    {
+        Devices = [new TwinCATDeviceOptions(
+            HostAddress: Gateway,
+            DeviceName: $"cli-{AmsNetId}:{AmsPort}")],
+        Tags = tags,
+        Timeout = Timeout,
+        Probe = new TwinCATProbeOptions { Enabled = false },
+        UseNativeNotifications = !PollOnly,
+        EnableControllerBrowse = false,
+    };
+
+    // ---- Test hook ----
+    internal TwinCATDriverOptions BuildOptionsForTest(IReadOnlyList<TwinCATTagDefinition> tags)
+        => BuildOptions(tags);
+}
@@ -0,0 +1,210 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="AbCipCommandBase"/>: the shared <c>BuildOptions</c> projection
+///     (driver-options mapping the four commands depend on), the <c>RejectStructure</c>
+///     guard, the <c>Timeout</c> override behaviour, and <c>TimeoutMs</c> validation.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class AbCipCommandBaseTests
+{
+    /// <summary>
+    ///     Local subclass that surfaces the protected helpers + properties under test.
+    /// </summary>
+    [CliFx.Attributes.Command("test")]
+    private sealed class TestableCommand : AbCipCommandBase
+    {
+        public AbCipDriverOptions InvokeBuildOptions(IReadOnlyList<AbCipTagDefinition> tags)
+            => BuildOptions(tags);
+
+        public string InvokeDriverInstanceId => DriverInstanceId;
+
+        public override ValueTask ExecuteAsync(CliFx.Infrastructure.IConsole console)
+            => ValueTask.CompletedTask;
+    }
+
+    private static AbCipTagDefinition SampleTag(string name = "Motor01") => new(
+        Name: name,
+        DeviceHostAddress: "ab://10.0.0.5/1,0",
+        TagPath: "Motor01",
+        DataType: AbCipDataType.DInt,
+        Writable: false);
+
+    [Fact]
+    public void BuildOptions_disables_probe_so_cli_does_not_race_operator_reads()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.InvokeBuildOptions([SampleTag()]);
+
+        options.Probe.Enabled.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_disables_controller_browse()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.InvokeBuildOptions([SampleTag()]);
+
+        options.EnableControllerBrowse.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_disables_alarm_projection()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.InvokeBuildOptions([SampleTag()]);
+
+        options.EnableAlarmProjection.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_produces_one_device_with_gateway_family_and_derived_name()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.CompactLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.InvokeBuildOptions([SampleTag()]);
+
+        options.Devices.Count.ShouldBe(1);
+        var device = options.Devices[0];
+        device.HostAddress.ShouldBe("ab://10.0.0.5/1,0");
+        device.PlcFamily.ShouldBe(AbCipPlcFamily.CompactLogix);
+        device.DeviceName.ShouldBe("cli-CompactLogix");
+    }
+
+    [Fact]
+    public void BuildOptions_passes_supplied_tag_list_verbatim()
+    {
+        var tags = new[] { SampleTag("t1"), SampleTag("t2") };
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.InvokeBuildOptions(tags);
+
+        options.Tags.Count.ShouldBe(2);
+        options.Tags[0].Name.ShouldBe("t1");
+        options.Tags[1].Name.ShouldBe("t2");
+    }
+
+    [Fact]
+    public void BuildOptions_carries_TimeoutMs_through_to_Timeout()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+            TimeoutMs = 7500,
+        };
+
+        var options = cmd.InvokeBuildOptions([SampleTag()]);
+
+        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500));
+    }
+
+    [Fact]
+    public void DriverInstanceId_embeds_gateway_for_log_disambiguation()
+    {
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Family = AbCipPlcFamily.ControlLogix,
+        };
+
+        cmd.InvokeDriverInstanceId.ShouldBe("abcip-cli-ab://10.0.0.5/1,0");
+    }
+
+    [Fact]
+    public void Timeout_setter_is_inert_and_does_not_silently_swallow_assignments()
+    {
+        // Driver.AbCip.Cli-006 — the empty init body would silently discard an
+        // object-initializer assignment, hiding a "driven by TimeoutMs" misuse. The fix
+        // makes it fail-fast with NotSupportedException so the contract is explicit.
+        Should.Throw<NotSupportedException>(() => new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            Timeout = TimeSpan.FromSeconds(99),
+        });
+    }
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    public void Timeout_get_throws_CommandException_when_TimeoutMs_is_non_positive(int badMs)
+    {
+        // Driver.AbCip.Cli-004 — TimeoutMs must be > 0. Validation is exposed via the
+        // Timeout getter so any command path that touches Timeout sees the same guard.
+        var cmd = new TestableCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            TimeoutMs = badMs,
+        };
+
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => _ = cmd.Timeout);
+        ex.Message.ShouldContain("--timeout-ms");
+    }
+
+    [Fact]
+    public void RejectStructure_throws_for_Structure_DataType()
+    {
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(
+            () => CallRejectStructure(AbCipDataType.Structure));
+        ex.Message.ShouldContain("Structure");
+    }
+
+    [Theory]
+    [InlineData(AbCipDataType.DInt)]
+    [InlineData(AbCipDataType.Bool)]
+    [InlineData(AbCipDataType.Real)]
+    public void RejectStructure_passes_for_atomic_types(AbCipDataType type)
+    {
+        // No throw — atomic types are allowed.
+        Should.NotThrow(() => CallRejectStructure(type));
+    }
+
+    // The static helper is protected; reflect to it once so the test stays at AbCipCommandBase.
+    private static void CallRejectStructure(AbCipDataType type)
+    {
+        var method = typeof(AbCipCommandBase).GetMethod(
+            "RejectStructure",
+            System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)
+            ?? throw new InvalidOperationException("RejectStructure not found");
+        try
+        {
+            method.Invoke(null, [type]);
+        }
+        catch (System.Reflection.TargetInvocationException tie) when (tie.InnerException is not null)
+        {
+            throw tie.InnerException;
+        }
+    }
+}
@@ -0,0 +1,34 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="SubscribeCommand.ValidateInterval(int)"/> — the guard that
+///     stops a zero / negative <c>--interval-ms</c> from reaching <c>SubscribeAsync</c>
+///     as a non-positive <see cref="TimeSpan"/>.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class SubscribeCommandIntervalTests
+{
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    [InlineData(-500)]
+    public void ValidateInterval_rejects_non_positive(int badMs)
+    {
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(
+            () => SubscribeCommand.ValidateInterval(badMs));
+        ex.Message.ShouldContain("--interval-ms");
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(250)]
+    [InlineData(60_000)]
+    public void ValidateInterval_accepts_positive(int goodMs)
+    {
+        Should.NotThrow(() => SubscribeCommand.ValidateInterval(goodMs));
+    }
+}
@@ -0,0 +1,134 @@
+using System;
+using System.Collections.Generic;
+using CliFx.Attributes;
+using CliFx.Infrastructure;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy;
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests;
+
+/// <summary>
+///     Locks in <see cref="AbLegacyCommandBase.BuildOptions"/>: probe disabled,
+///     device shape populated from <c>--gateway</c> + <c>--plc-type</c>, tag list
+///     forwarded verbatim, and timeout propagated from <c>--timeout-ms</c>. A
+///     regression here silently changes every AbLegacy CLI command's behaviour, so
+///     covering it explicitly closes the test gap called out by finding
+///     Driver.AbLegacy.Cli-007.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class BuildOptionsTests
+{
+    // Concrete subclass needed because AbLegacyCommandBase is abstract. Exposes the
+    // protected BuildOptions via a public surface for the test.
+    // [Command] satisfies CliFx's analyzer (ICommand subtypes must be annotated);
+    // we never run it through CliFx, only invoke Build() directly.
+    [Command("test-build-options")]
+    private sealed class TestCommand : AbLegacyCommandBase
+    {
+        public AbLegacyDriverOptions Build(IReadOnlyList<AbLegacyTagDefinition> tags)
+            => BuildOptions(tags);
+
+        public override System.Threading.Tasks.ValueTask ExecuteAsync(IConsole console)
+            => throw new NotSupportedException("TestCommand is for BuildOptions inspection only.");
+    }
+
+    private static readonly IReadOnlyList<AbLegacyTagDefinition> SampleTags =
+    [
+        new AbLegacyTagDefinition(
+            Name: "N7:0:Int",
+            DeviceHostAddress: "ab://192.168.1.20/1,0",
+            Address: "N7:0",
+            DataType: AbLegacyDataType.Int,
+            Writable: false),
+        new AbLegacyTagDefinition(
+            Name: "F8:0:Float",
+            DeviceHostAddress: "ab://192.168.1.20/1,0",
+            Address: "F8:0",
+            DataType: AbLegacyDataType.Float,
+            Writable: true),
+    ];
+
+    [Fact]
+    public void BuildOptions_disables_probe_for_cli_oneshot_runs()
+    {
+        var cmd = new TestCommand
+        {
+            Gateway = "ab://192.168.1.20/1,0",
+            PlcType = AbLegacyPlcFamily.Slc500,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.Build(SampleTags);
+
+        options.Probe.ShouldNotBeNull();
+        options.Probe.Enabled.ShouldBeFalse(
+            "CLI commands are one-shot; the background probe loop is unwanted overhead.");
+    }
+
+    [Fact]
+    public void BuildOptions_populates_single_device_from_gateway_and_plc_type()
+    {
+        var cmd = new TestCommand
+        {
+            Gateway = "ab://10.0.0.5/1,0",
+            PlcType = AbLegacyPlcFamily.MicroLogix,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.Build(SampleTags);
+
+        options.Devices.Count.ShouldBe(1);
+        options.Devices[0].HostAddress.ShouldBe("ab://10.0.0.5/1,0");
+        options.Devices[0].PlcFamily.ShouldBe(AbLegacyPlcFamily.MicroLogix);
+        options.Devices[0].DeviceName.ShouldBe("cli-MicroLogix");
+    }
+
+    [Fact]
+    public void BuildOptions_forwards_tag_list_verbatim()
+    {
+        var cmd = new TestCommand
+        {
+            Gateway = "ab://192.168.1.20/1,0",
+            PlcType = AbLegacyPlcFamily.Slc500,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.Build(SampleTags);
+
+        options.Tags.ShouldBe(SampleTags);
+    }
+
+    [Fact]
+    public void BuildOptions_propagates_timeout_ms()
+    {
+        var cmd = new TestCommand
+        {
+            Gateway = "ab://192.168.1.20/1,0",
+            PlcType = AbLegacyPlcFamily.Slc500,
+            TimeoutMs = 7500,
+        };
+
+        var options = cmd.Build(SampleTags);
+
+        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500));
+    }
+
+    [Fact]
+    public void BuildOptions_with_empty_tag_list_yields_empty_tags_collection()
+    {
+        var cmd = new TestCommand
+        {
+            Gateway = "ab://192.168.1.20/1,0",
+            PlcType = AbLegacyPlcFamily.Plc5,
+            TimeoutMs = 5000,
+        };
+
+        var options = cmd.Build([]);
+
+        options.Tags.ShouldBeEmpty();
+        options.Devices.Count.ShouldBe(1);
+        options.Devices[0].PlcFamily.ShouldBe(AbLegacyPlcFamily.Plc5);
+    }
+}
@@ -0,0 +1,80 @@
+using System.Linq;
+using System.Reflection;
+using CliFx.Attributes;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests;
+
+/// <summary>
+///     Locks in the CLI command-option contract surface area — short aliases and
+///     help-text wording — that the AbLegacy CLI is expected to keep in parity with
+///     its sibling AbCip CLI and with <c>docs/Driver.AbLegacy.Cli.md</c>.
+///     Regression coverage for findings Driver.AbLegacy.Cli-002, -005, -006.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class CommandMetadataTests
+{
+    private static CommandOptionAttribute GetOption<TCommand>(string propertyName)
+    {
+        var prop = typeof(TCommand).GetProperty(
+            propertyName,
+            BindingFlags.Public | BindingFlags.Instance);
+        prop.ShouldNotBeNull($"property {propertyName} is missing from {typeof(TCommand).Name}");
+        var attr = prop!.GetCustomAttribute<CommandOptionAttribute>();
+        attr.ShouldNotBeNull(
+            $"property {propertyName} on {typeof(TCommand).Name} lacks [CommandOption]");
+        return attr!;
+    }
+
+    // ---------- Driver.AbLegacy.Cli-006 — ProbeCommand --type needs short alias 't' ----------
+
+    [Fact]
+    public void ProbeCommand_type_has_short_alias_t()
+    {
+        // Parity with read / write / subscribe: --type / -t works everywhere.
+        var attr = GetOption<ProbeCommand>(nameof(ProbeCommand.DataType));
+        attr.ShortName.ShouldBe('t');
+    }
+
+    [Theory]
+    [InlineData(typeof(ReadCommand), nameof(ReadCommand.DataType))]
+    [InlineData(typeof(WriteCommand), nameof(WriteCommand.DataType))]
+    [InlineData(typeof(SubscribeCommand), nameof(SubscribeCommand.DataType))]
+    public void Other_commands_keep_type_short_alias_t(System.Type commandType, string propName)
+    {
+        var prop = commandType.GetProperty(propName, BindingFlags.Public | BindingFlags.Instance);
+        prop.ShouldNotBeNull();
+        var attr = prop!.GetCustomAttribute<CommandOptionAttribute>();
+        attr.ShouldNotBeNull();
+        attr!.ShortName.ShouldBe('t');
+    }
+
+    // ---------- Driver.AbLegacy.Cli-002 — WriteCommand --value help lists full bool alias set ----------
+
+    [Fact]
+    public void WriteCommand_value_help_lists_full_boolean_alias_set()
+    {
+        // ParseBool accepts true/false, 1/0, on/off, yes/no — the help text must say so
+        // (DriverClis.md documents the full alias set as the shared CLI contract).
+        var attr = GetOption<WriteCommand>(nameof(WriteCommand.Value));
+        attr.Description.ShouldNotBeNull();
+        attr.Description!.ShouldContain("true/false", Case.Insensitive);
+        attr.Description!.ShouldContain("1/0");
+        attr.Description!.ShouldContain("on/off", Case.Insensitive);
+        attr.Description!.ShouldContain("yes/no", Case.Insensitive);
+    }
+
+    // ---------- Driver.AbLegacy.Cli-005 — SubscribeCommand --interval-ms help notes 250ms floor ----------
+
+    [Fact]
+    public void SubscribeCommand_interval_ms_help_notes_PollGroupEngine_floor()
+    {
+        // Parity with AbCip CLI: operators passing -i 100 deserve a heads-up that
+        // PollGroupEngine floors sub-250ms values.
+        var attr = GetOption<SubscribeCommand>(nameof(SubscribeCommand.IntervalMs));
+        attr.Description.ShouldNotBeNull();
+        attr.Description!.ShouldContain("250", Case.Insensitive);
+    }
+}
@@ -0,0 +1,66 @@
+using CliFx.Infrastructure;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests;
+
+/// <summary>
+///     Covers Driver.Modbus.Cli-005: <c>probe</c> / <c>read</c> / <c>write</c> must swallow
+///     <see cref="OperationCanceledException"/> so a Ctrl+C during InitializeAsync exits
+///     cleanly instead of dumping a full stack trace through CliFx. <c>SubscribeCommand</c>
+///     already handles this around its <c>Task.Delay</c>; these tests pin the same behaviour
+///     to the connect/read/write commands.
+///     The test pre-cancels the CliFx <see cref="FakeInMemoryConsole"/>; the driver's
+///     <c>ConnectAsync</c> observes the token via <c>Dns.GetHostAddressesAsync</c> and throws
+///     OCE before any socket I/O happens, so the test is hermetic — no real PLC needed.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class CommandCancellationTests
+{
+    [Fact]
+    public async Task ProbeCommand_swallows_cancellation_during_initialize()
+    {
+        using var console = new FakeInMemoryConsole();
+        console.RequestCancellation(); // simulate Ctrl+C before ExecuteAsync runs
+
+        var sut = new ProbeCommand { Host = "127.0.0.1" };
+
+        await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console));
+    }
+
+    [Fact]
+    public async Task ReadCommand_swallows_cancellation_during_initialize()
+    {
+        using var console = new FakeInMemoryConsole();
+        console.RequestCancellation();
+
+        var sut = new ReadCommand
+        {
+            Host = "127.0.0.1",
+            Region = ModbusRegion.HoldingRegisters,
+            Address = 0,
+            DataType = ModbusDataType.UInt16,
+        };
+
+        await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console));
+    }
+
+    [Fact]
+    public async Task WriteCommand_swallows_cancellation_during_initialize()
+    {
+        using var console = new FakeInMemoryConsole();
+        console.RequestCancellation();
+
+        var sut = new WriteCommand
+        {
+            Host = "127.0.0.1",
+            Region = ModbusRegion.HoldingRegisters,
+            Address = 0,
+            DataType = ModbusDataType.UInt16,
+            Value = "42",
+        };
+
+        await Should.NotThrowAsync(async () => await sut.ExecuteAsync(console));
+    }
+}
@@ -0,0 +1,164 @@
+using CliFx.Attributes;
+using CliFx.Infrastructure;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.Cli.Common;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="ModbusCommandBase.BuildOptions"/> — the pure, deterministic mapping
+///     from the base's host / port / unit-id / timeout / disable-reconnect flags onto a
+///     <c>ModbusDriverOptions</c>. The CLI is one-shot so the background connectivity probe
+///     must be disabled; <c>AutoReconnect</c> is the inverse of <c>--disable-reconnect</c>.
+///     Also covers the input-range validation introduced for Driver.Modbus.Cli-003.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class ModbusCommandBaseTests
+{
+    // Test-only ModbusCommandBase concrete subclass that exposes the protected BuildOptions
+    // helper + ValidateEndpoint. The [Command] attribute is required by the CliFx analyzer
+    // (CliFx_CommandMustBeAnnotated) — this command is never registered with the CLI app
+    // but the analyzer rule fires for every ICommand implementor in the compilation.
+    [Command("noop-test", Description = "Test-only probe of ModbusCommandBase.BuildOptions.")]
+    private sealed class ProbeOnly : ModbusCommandBase
+    {
+        public override ValueTask ExecuteAsync(IConsole console) => default;
+        public ModbusDriverOptions Invoke(IReadOnlyList<ModbusTagDefinition> tags) => BuildOptions(tags);
+        public void InvokeValidate() => ValidateEndpoint();
+    }
+
+    [Fact]
+    public void BuildOptions_disables_probe_for_one_shot_cli_runs()
+    {
+        var sut = new ProbeOnly { Host = "10.0.0.5" };
+
+        var options = sut.Invoke([]);
+
+        options.Probe.ShouldNotBeNull();
+        options.Probe.Enabled.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_maps_TimeoutMs_to_Timeout_TimeSpan()
+    {
+        var sut = new ProbeOnly { Host = "h", TimeoutMs = 7500 };
+
+        var options = sut.Invoke([]);
+
+        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500));
+    }
+
+    [Fact]
+    public void BuildOptions_AutoReconnect_defaults_to_true_when_flag_unset()
+    {
+        var sut = new ProbeOnly { Host = "h" };
+
+        var options = sut.Invoke([]);
+
+        options.AutoReconnect.ShouldBeTrue();
+    }
+
+    [Fact]
+    public void BuildOptions_AutoReconnect_becomes_false_when_disable_reconnect_flag_set()
+    {
+        var sut = new ProbeOnly { Host = "h", DisableAutoReconnect = true };
+
+        var options = sut.Invoke([]);
+
+        options.AutoReconnect.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_flows_host_port_unit_through()
+    {
+        var sut = new ProbeOnly { Host = "plc.shop.local", Port = 5020, UnitId = 17, TimeoutMs = 3000 };
+
+        var options = sut.Invoke([]);
+
+        options.Host.ShouldBe("plc.shop.local");
+        options.Port.ShouldBe(5020);
+        options.UnitId.ShouldBe((byte)17);
+    }
+
+    [Fact]
+    public void BuildOptions_forwards_tag_list_verbatim()
+    {
+        var sut = new ProbeOnly { Host = "h" };
+        var tag = new ModbusTagDefinition(
+            Name: "T", Region: ModbusRegion.HoldingRegisters, Address: 0, DataType: ModbusDataType.UInt16);
+
+        var options = sut.Invoke([tag]);
+
+        options.Tags.Count.ShouldBe(1);
+        options.Tags[0].ShouldBeSameAs(tag);
+    }
+
+    // --- Driver.Modbus.Cli-003: parse-time endpoint validation -------------------------------
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    [InlineData(65536)]
+    [InlineData(int.MinValue)]
+    [InlineData(int.MaxValue)]
+    public void ValidateEndpoint_rejects_port_outside_1_to_65535(int port)
+    {
+        var sut = new ProbeOnly { Host = "h", Port = port };
+
+        Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(502)]
+    [InlineData(65535)]
+    public void ValidateEndpoint_accepts_port_in_range(int port)
+    {
+        var sut = new ProbeOnly { Host = "h", Port = port };
+
+        Should.NotThrow(() => sut.InvokeValidate());
+    }
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    [InlineData(-2000)]
+    public void ValidateEndpoint_rejects_non_positive_timeout(int timeoutMs)
+    {
+        var sut = new ProbeOnly { Host = "h", TimeoutMs = timeoutMs };
+
+        Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
+    }
+
+    [Theory]
+    [InlineData(0)]   // broadcast — disallowed for unicast read/write requests
+    [InlineData(248)]
+    [InlineData(255)]
+    public void ValidateEndpoint_rejects_unit_id_outside_1_to_247(byte unitId)
+    {
+        var sut = new ProbeOnly { Host = "h", UnitId = unitId };
+
+        Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(247)]
+    [InlineData(50)]
+    public void ValidateEndpoint_accepts_unit_id_in_range(byte unitId)
+    {
+        var sut = new ProbeOnly { Host = "h", UnitId = unitId };
+
+        Should.NotThrow(() => sut.InvokeValidate());
+    }
+
+    [Fact]
+    public void ValidateEndpoint_accepts_default_options()
+    {
+        // Defaults: Port=502, UnitId=1, TimeoutMs=2000. All inside the valid ranges.
+        var sut = new ProbeOnly { Host = "h" };
+
+        Should.NotThrow(() => sut.InvokeValidate());
+    }
+}
@@ -0,0 +1,60 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="ProbeCommand.ComputeVerdict"/> — the headline-string helper that
+///     combines the driver-side <see cref="DriverState"/> with the probe snapshot's OPC UA
+///     <see cref="DataValueSnapshot.StatusCode"/> so the operator never sees the previous
+///     contradictory pair (`Health: Healthy` above a `Status: Bad...` snapshot line — see
+///     Driver.Modbus.Cli-006).
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class ProbeCommandTests
+{
+    [Fact]
+    public void ComputeVerdict_returns_OK_when_state_is_Healthy_and_status_is_Good()
+    {
+        var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x00000000u);
+
+        verdict.ShouldContain("OK");
+    }
+
+    [Theory]
+    [InlineData(DriverState.Faulted)]
+    [InlineData(DriverState.Reconnecting)]
+    [InlineData(DriverState.Unknown)]
+    public void ComputeVerdict_returns_FAIL_when_driver_state_is_not_Healthy(DriverState state)
+    {
+        var verdict = ProbeCommand.ComputeVerdict(state, statusCode: 0x00000000u);
+
+        verdict.ShouldContain("FAIL");
+    }
+
+    [Theory]
+    [InlineData(0x80050000u)] // BadCommunicationError
+    [InlineData(0x800A0000u)] // BadTimeout
+    [InlineData(0x80740000u)] // BadTypeMismatch
+    [InlineData(0x80000000u)] // generic Bad
+    public void ComputeVerdict_returns_FAIL_when_snapshot_status_is_Bad_even_if_driver_state_Healthy(uint statusCode)
+    {
+        // Driver.Modbus.Cli-006: a successful InitializeAsync sets DriverState.Healthy, but
+        // the FC03 probe read may still fail (snapshot.StatusCode != Good). Previously the
+        // headline reported Healthy while the snapshot line below showed Bad. The verdict
+        // must reflect the actual probe-read outcome.
+        var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode);
+
+        verdict.ShouldContain("FAIL");
+    }
+
+    [Fact]
+    public void ComputeVerdict_returns_DEGRADED_for_uncertain_status_with_healthy_driver()
+    {
+        var verdict = ProbeCommand.ComputeVerdict(DriverState.Healthy, statusCode: 0x40000000u);
+
+        verdict.ShouldContain("DEGRADED");
+    }
+}
@@ -0,0 +1,63 @@
+using CliFx.Infrastructure;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Cli.Tests;
+
+/// <summary>
+///     Covers the branch validation inside <see cref="WriteCommand.ExecuteAsync"/>:
+///     1. Driver.Modbus.Cli-002 — write to <see cref="ModbusRegion.Coils"/> must use
+///        <c>--type Bool</c>.
+///     2. Read-only regions (DiscreteInputs / InputRegisters) reject any write.
+///     The actual driver call is never reached for these guard cases — they throw a
+///     <see cref="CliFx.Exceptions.CommandException"/> before the driver is constructed,
+///     so we can exercise <c>ExecuteAsync</c> against an unreachable host.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class WriteCommandRegionValidationTests
+{
+    [Theory]
+    [InlineData(ModbusRegion.DiscreteInputs, ModbusDataType.Bool, "0")]
+    [InlineData(ModbusRegion.InputRegisters, ModbusDataType.UInt16, "1")]
+    public async Task ExecuteAsync_rejects_read_only_regions(
+        ModbusRegion region, ModbusDataType type, string value)
+    {
+        var sut = new WriteCommand
+        {
+            // Host is required, but the guard fires before any socket use.
+            Host = "127.0.0.1",
+            Region = region,
+            Address = 0,
+            DataType = type,
+            Value = value,
+        };
+        using var console = new FakeInMemoryConsole();
+
+        await Should.ThrowAsync<CliFx.Exceptions.CommandException>(
+            async () => await sut.ExecuteAsync(console));
+    }
+
+    [Theory]
+    [InlineData(ModbusDataType.UInt16)]
+    [InlineData(ModbusDataType.Int16)]
+    [InlineData(ModbusDataType.Float32)]
+    [InlineData(ModbusDataType.Int32)]
+    public async Task ExecuteAsync_rejects_non_Bool_type_for_Coils_region(ModbusDataType type)
+    {
+        var sut = new WriteCommand
+        {
+            Host = "127.0.0.1",
+            Region = ModbusRegion.Coils,
+            Address = 5,
+            DataType = type,
+            Value = "42",
+        };
+        using var console = new FakeInMemoryConsole();
+
+        var ex = await Should.ThrowAsync<CliFx.Exceptions.CommandException>(
+            async () => await sut.ExecuteAsync(console));
+        ex.Message.ShouldContain("Coils");
+        ex.Message.ShouldContain("Bool");
+    }
+}
@@ -0,0 +1,61 @@
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests;
+
+/// <summary>
+///     Driver.S7.Cli-004: every S7 CLI command must own one disposal mechanism for the
+///     <c>S7Driver</c>, not two. The chosen mechanism is <c>await using var driver = ...</c>
+///     — <c>S7Driver.DisposeAsync</c> already calls <c>ShutdownAsync</c>, so an additional
+///     explicit <c>driver.ShutdownAsync(...)</c> in a <c>finally</c> block runs shutdown
+///     twice (three times on subscribe). These tests guard against that regression by
+///     scanning the command source files.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class CommandDisposalConventionsTests
+{
+    private static readonly string CommandsDir = LocateCommandsDir();
+
+    [Theory]
+    [InlineData("ProbeCommand.cs")]
+    [InlineData("ReadCommand.cs")]
+    [InlineData("WriteCommand.cs")]
+    [InlineData("SubscribeCommand.cs")]
+    public void Command_does_not_call_ShutdownAsync_explicitly(string commandFile)
+    {
+        var path = Path.Combine(CommandsDir, commandFile);
+        File.Exists(path).ShouldBeTrue($"Expected {path} to exist.");
+        var source = File.ReadAllText(path);
+
+        // The await-using statement is the single disposal mechanism. An explicit
+        // driver.ShutdownAsync(...) call (typically inside a finally block) re-invokes
+        // a shutdown path that DisposeAsync already runs and is the smell -004 flags.
+        source.ShouldNotContain("driver.ShutdownAsync(");
+    }
+
+    [Theory]
+    [InlineData("ProbeCommand.cs")]
+    [InlineData("ReadCommand.cs")]
+    [InlineData("WriteCommand.cs")]
+    [InlineData("SubscribeCommand.cs")]
+    public void Command_uses_await_using_for_S7Driver(string commandFile)
+    {
+        var path = Path.Combine(CommandsDir, commandFile);
+        var source = File.ReadAllText(path);
+
+        source.ShouldContain("await using var driver = new S7Driver(");
+    }
+
+    private static string LocateCommandsDir()
+    {
+        // Walk up from the test assembly bin/ folder to the repo root, then into the
+        // source project's Commands/ directory. The test-host puts CWD somewhere under
+        // bin/Debug/net10.0 so we resolve relative to AppContext.BaseDirectory.
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx")))
+            dir = dir.Parent;
+        dir.ShouldNotBeNull("Could not find solution root (ZB.MOM.WW.OtOpcUa.slnx).");
+        return Path.Combine(
+            dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.S7.Cli", "Commands");
+    }
+}
@@ -0,0 +1,92 @@
+using CliFx.Attributes;
+using CliFx.Infrastructure;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.S7.Cli;
+using S7NetCpuType = global::S7.Net.CpuType;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="S7CommandBase.BuildOptions"/> — the pure, deterministic mapping
+///     from the base's host/port/CPU/rack/slot/timeout flags onto an
+///     <c>S7DriverOptions</c>. The CLI is one-shot so the background connectivity probe
+///     must be disabled.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class S7CommandBaseBuildOptionsTests
+{
+    // Test-only S7CommandBase concrete subclass that exposes the protected BuildOptions
+    // helper. The [Command] attribute is required by the CliFx analyzer
+    // (CliFx_CommandMustBeAnnotated) — this command is never registered with the CLI app
+    // but the analyzer rule fires for every ICommand implementor in the compilation.
+    [Command("noop-test", Description = "Test-only probe of S7CommandBase.BuildOptions.")]
+    private sealed class ProbeOnly : S7CommandBase
+    {
+        public override ValueTask ExecuteAsync(IConsole console) => default;
+        public S7DriverOptions Invoke(IReadOnlyList<S7TagDefinition> tags) => BuildOptions(tags);
+    }
+
+    [Fact]
+    public void BuildOptions_disables_probe_for_one_shot_cli_runs()
+    {
+        var sut = new ProbeOnly
+        {
+            Host = "10.0.0.5",
+            Port = 102,
+            CpuType = S7NetCpuType.S71500,
+            Rack = 0,
+            Slot = 0,
+            TimeoutMs = 5000,
+        };
+
+        var options = sut.Invoke([]);
+
+        options.Probe.ShouldNotBeNull();
+        options.Probe.Enabled.ShouldBeFalse();
+    }
+
+    [Fact]
+    public void BuildOptions_maps_TimeoutMs_to_Timeout_TimeSpan()
+    {
+        var sut = new ProbeOnly { Host = "h", TimeoutMs = 7500 };
+
+        var options = sut.Invoke([]);
+
+        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500));
+    }
+
+    [Fact]
+    public void BuildOptions_flows_host_port_cpu_rack_slot_through()
+    {
+        var sut = new ProbeOnly
+        {
+            Host = "plc.shop.local",
+            Port = 4102,
+            CpuType = S7NetCpuType.S7300,
+            Rack = 1,
+            Slot = 2,
+            TimeoutMs = 3000,
+        };
+
+        var options = sut.Invoke([]);
+
+        options.Host.ShouldBe("plc.shop.local");
+        options.Port.ShouldBe(4102);
+        options.CpuType.ShouldBe(S7NetCpuType.S7300);
+        options.Rack.ShouldBe((short)1);
+        options.Slot.ShouldBe((short)2);
+    }
+
+    [Fact]
+    public void BuildOptions_forwards_tag_list_verbatim()
+    {
+        var sut = new ProbeOnly { Host = "h" };
+        var tag = new S7TagDefinition("t", "MW0", S7DataType.Int16, Writable: false);
+
+        var options = sut.Invoke([tag]);
+
+        options.Tags.Count.ShouldBe(1);
+        options.Tags[0].ShouldBeSameAs(tag);
+    }
+}
@@ -0,0 +1,32 @@
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests;
+
+/// <summary>
+///     Driver.S7.Cli-007: the S7 subscribe command — a near-verbatim copy of the Modbus
+///     subscribe command — must keep the comment that explains why <c>OnDataChange</c>
+///     uses <c>console.Output.WriteLine</c> (synchronous, on a driver background thread)
+///     instead of <c>System.Console</c> or the async <c>WriteLineAsync</c>. The rationale
+///     is non-obvious to a reader and the Modbus copy carries it; the S7 copy must too.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class SubscribeCommandConsoleHandlerCommentTests
+{
+    [Fact]
+    public void SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously()
+    {
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx")))
+            dir = dir.Parent;
+        dir.ShouldNotBeNull();
+        var source = File.ReadAllText(Path.Combine(
+            dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.S7.Cli",
+            "Commands", "SubscribeCommand.cs"));
+
+        // The comment must reference the CliFx console abstraction so future copy-pastes
+        // do not lose the rationale.
+        source.ShouldContain("CliFx console");
+        source.ShouldContain("IConsole");
+    }
+}
@@ -0,0 +1,123 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests;
+
+/// <summary>
+///     Driver.TwinCAT.Cli-006: covers the prefix / max filtering and the RO/RW classification
+///     logic inside <see cref="BrowseCommand"/>. The collecting builder + the filter pipeline
+///     are pure — no ADS contact required — so they unit-test cleanly.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class BrowseCommandFilterTests
+{
+    private static DriverAttributeInfo Info(SecurityClassification cls, DriverDataType dt = DriverDataType.Int32)
+        => new(
+            FullName: "ignored",
+            DriverDataType: dt,
+            IsArray: false,
+            ArrayDim: null,
+            SecurityClass: cls,
+            IsHistorized: false);
+
+    [Fact]
+    public void Collector_records_each_variable_in_call_order()
+    {
+        var builder = new BrowseCommand.CollectingAddressSpaceBuilder();
+        builder.Variable("GVL.A", "GVL.A", Info(SecurityClassification.Operate));
+        builder.Variable("GVL.B", "GVL.B", Info(SecurityClassification.ViewOnly));
+
+        builder.Variables.Count.ShouldBe(2);
+        builder.Variables[0].BrowseName.ShouldBe("GVL.A");
+        builder.Variables[1].BrowseName.ShouldBe("GVL.B");
+    }
+
+    [Fact]
+    public void Folder_returns_same_builder_so_nested_variables_land_in_one_flat_list()
+    {
+        // BrowseCommand expects a flat list — TwinCAT's flat-mode symbol walk doesn't nest
+        // into sub-folders, but DiscoverAsync may still call Folder() before Variable().
+        var builder = new BrowseCommand.CollectingAddressSpaceBuilder();
+        var nested = builder.Folder("Discovered", "Discovered");
+        nested.Variable("GVL.X", "GVL.X", Info(SecurityClassification.Operate));
+
+        builder.Variables.Count.ShouldBe(1);
+        builder.Variables[0].BrowseName.ShouldBe("GVL.X");
+    }
+
+    [Fact]
+    public void FilterAndLimit_empty_prefix_returns_everything_up_to_max()
+    {
+        var symbols = new List<(string BrowseName, DriverAttributeInfo Info)>
+        {
+            ("GVL_A.x", Info(SecurityClassification.Operate)),
+            ("GVL_B.y", Info(SecurityClassification.ViewOnly)),
+            ("MAIN.z",  Info(SecurityClassification.Operate)),
+        };
+
+        var matched = BrowseCommand.FilterByPrefix(symbols, prefix: null);
+        matched.Count.ShouldBe(3);
+    }
+
+    [Fact]
+    public void FilterAndLimit_prefix_is_case_sensitive()
+    {
+        var symbols = new List<(string BrowseName, DriverAttributeInfo Info)>
+        {
+            ("GVL_Fixture.x", Info(SecurityClassification.Operate)),
+            ("gvl_fixture.y", Info(SecurityClassification.Operate)),
+            ("MAIN.z",        Info(SecurityClassification.Operate)),
+        };
+
+        var matched = BrowseCommand.FilterByPrefix(symbols, prefix: "GVL_Fixture");
+
+        matched.Count.ShouldBe(1);
+        matched[0].BrowseName.ShouldBe("GVL_Fixture.x");
+    }
+
+    [Fact]
+    public void FilterAndLimit_zero_max_means_unbounded()
+    {
+        var symbols = Enumerable.Range(0, 10)
+            .Select(i => ($"GVL.S{i}", Info(SecurityClassification.Operate)))
+            .ToList();
+
+        var limit = BrowseCommand.PrintLimit(symbols.Count, max: 0);
+        limit.ShouldBe(10);
+    }
+
+    [Fact]
+    public void FilterAndLimit_caps_to_max_when_more_matched()
+    {
+        BrowseCommand.PrintLimit(matchedCount: 1000, max: 50).ShouldBe(50);
+    }
+
+    [Fact]
+    public void FilterAndLimit_does_not_pad_to_max_when_fewer_matched()
+    {
+        BrowseCommand.PrintLimit(matchedCount: 3, max: 50).ShouldBe(3);
+    }
+
+    [Fact]
+    public void AccessTag_returns_RO_for_ViewOnly_attribute()
+    {
+        BrowseCommand.AccessTag(Info(SecurityClassification.ViewOnly)).ShouldBe("RO");
+    }
+
+    [Theory]
+    [InlineData(SecurityClassification.FreeAccess)]
+    [InlineData(SecurityClassification.Operate)]
+    [InlineData(SecurityClassification.SecuredWrite)]
+    [InlineData(SecurityClassification.VerifiedWrite)]
+    [InlineData(SecurityClassification.Tune)]
+    [InlineData(SecurityClassification.Configure)]
+    public void AccessTag_returns_RW_for_anything_except_ViewOnly(SecurityClassification cls)
+    {
+        // BrowseCommand's display logic flips ViewOnly = RO, everything else = RW. The real
+        // ACL is enforced server-side from the SecurityClassification — the CLI label is just
+        // a coarse "is this writable from any tier" indicator.
+        BrowseCommand.AccessTag(Info(cls)).ShouldBe("RW");
+    }
+}
@@ -0,0 +1,37 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests;
+
+/// <summary>
+///     Driver.TwinCAT.Cli-003: the subscribe banner mechanism label is derived from the
+///     <see cref="ISubscriptionHandle.DiagnosticId"/> the driver actually returned, not from
+///     the <c>--poll-only</c> flag. That way the banner cannot disagree with what the driver
+///     did even if a future fallback path lands the subscription somewhere unexpected.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class SubscribeCommandMechanismTests
+{
+    private sealed record StubHandle(string DiagnosticId) : ISubscriptionHandle;
+
+    [Theory]
+    [InlineData("twincat-native-sub-1")]
+    [InlineData("twincat-native-sub-42")]
+    [InlineData("twincat-native-sub-9223372036854775807")]
+    public void DescribeMechanism_returns_ADS_notification_for_native_handle(string diagId)
+    {
+        SubscribeCommand.DescribeMechanism(new StubHandle(diagId)).ShouldBe("ADS notification");
+    }
+
+    [Theory]
+    [InlineData("pollgroup-1")]
+    [InlineData("modbus-poll-7")]
+    [InlineData("")]
+    [InlineData("TWINCAT-NATIVE-SUB-1")] // ordinal comparison — uppercase prefix does NOT match.
+    public void DescribeMechanism_returns_polling_for_anything_else(string diagId)
+    {
+        SubscribeCommand.DescribeMechanism(new StubHandle(diagId)).ShouldBe("polling");
+    }
+}
@@ -0,0 +1,238 @@
+using System.Reflection;
+using CliFx.Attributes;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Commands;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests;
+
+/// <summary>
+///     Covers <see cref="TwinCATCommandBase"/> / <see cref="TwinCATTagCommandBase"/> wiring:
+///     the canonical gateway string, the driver instance id, the BuildOptions field projection
+///     (Driver.TwinCAT.Cli-006), and the up-front range validation guards
+///     (Driver.TwinCAT.Cli-001).
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class TwinCATCommandBaseTests
+{
+    [Fact]
+    public void Gateway_uses_canonical_ads_scheme_with_port()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "192.168.1.40.1.1",
+            AmsPort = 851,
+            SymbolPath = "MAIN.bRunning",
+        };
+        cmd.GatewayForTest.ShouldBe("ads://192.168.1.40.1.1:851");
+    }
+
+    [Fact]
+    public void Gateway_round_trips_through_TwinCATAmsAddress_TryParse()
+    {
+        // Driver.TwinCAT.Cli-006: a regression in the Gateway string breaks every command
+        // because the driver's TwinCATAmsAddress.TryParse refuses anything not shaped
+        // ads://{netId}:{port}.
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "5.23.91.23.1.1",
+            AmsPort = 852,
+            SymbolPath = "MAIN.x",
+        };
+        var parsed = TwinCAT.TwinCATAmsAddress.TryParse(cmd.GatewayForTest);
+        parsed.ShouldNotBeNull();
+        parsed!.NetId.ShouldBe("5.23.91.23.1.1");
+        parsed.Port.ShouldBe(852);
+    }
+
+    [Fact]
+    public void DriverInstanceId_includes_ams_target()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            AmsPort = 851,
+            SymbolPath = "MAIN.x",
+        };
+        cmd.DriverInstanceIdForTest.ShouldBe("twincat-cli-127.0.0.1.1.1:851");
+    }
+
+    [Fact]
+    public void Timeout_is_projection_of_TimeoutMs_and_init_is_noop()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            TimeoutMs = 7777,
+            SymbolPath = "MAIN.x",
+        };
+        cmd.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7777));
+    }
+
+    [Fact]
+    public void BuildOptions_wires_device_tags_timeout_and_disables_probe()
+    {
+        // Driver.TwinCAT.Cli-006: cover the property-by-property wiring that the four runtime
+        // commands depend on. Probe must be disabled (CLI is one-shot — the probe loop would
+        // race the operator's own reads) and controller-browse must stay off.
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "10.0.0.1.1.1",
+            AmsPort = 851,
+            TimeoutMs = 4321,
+            SymbolPath = "MAIN.x",
+        };
+        var tag = new TwinCAT.TwinCATTagDefinition(
+            Name: "n1",
+            DeviceHostAddress: cmd.GatewayForTest,
+            SymbolPath: "MAIN.x",
+            DataType: TwinCAT.TwinCATDataType.DInt,
+            Writable: false);
+
+        var options = cmd.BuildOptionsForTest([tag]);
+
+        options.Devices.Count.ShouldBe(1);
+        options.Devices[0].HostAddress.ShouldBe("ads://10.0.0.1.1.1:851");
+        options.Devices[0].DeviceName.ShouldBe("cli-10.0.0.1.1.1:851");
+        options.Tags.ShouldBe([tag]);
+        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(4321));
+        options.Probe.Enabled.ShouldBeFalse();
+        options.EnableControllerBrowse.ShouldBeFalse();
+        // Default UseNativeNotifications = true (no --poll-only).
+        options.UseNativeNotifications.ShouldBeTrue();
+    }
+
+    [Fact]
+    public void BuildOptions_PollOnly_flips_UseNativeNotifications_off()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "10.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            PollOnly = true,
+        };
+        cmd.BuildOptionsForTest([]).UseNativeNotifications.ShouldBeFalse();
+    }
+
+    // ---- Driver.TwinCAT.Cli-001 (range validation) ----
+
+    [Fact]
+    public void Validate_rejects_zero_timeout()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            TimeoutMs = 0,
+        };
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => cmd.ValidateForTest());
+        ex.Message.ShouldContain("--timeout-ms");
+    }
+
+    [Fact]
+    public void Validate_rejects_negative_timeout()
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            TimeoutMs = -1,
+        };
+        Should.Throw<CliFx.Exceptions.CommandException>(() => cmd.ValidateForTest());
+    }
+
+    [Theory]
+    [InlineData(0)]
+    [InlineData(-1)]
+    [InlineData(65536)]
+    [InlineData(100000)]
+    public void Validate_rejects_out_of_range_ams_port(int port)
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            AmsPort = port,
+        };
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => cmd.ValidateForTest());
+        ex.Message.ShouldContain("--ams-port");
+    }
+
+    [Theory]
+    [InlineData(1)]
+    [InlineData(801)]
+    [InlineData(851)]
+    [InlineData(65535)]
+    public void Validate_accepts_in_range_ams_port(int port)
+    {
+        var cmd = new ProbeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            AmsPort = port,
+        };
+        Should.NotThrow(() => cmd.ValidateForTest());
+    }
+
+    [Fact]
+    public void SubscribeCommand_validate_rejects_zero_interval()
+    {
+        var cmd = new SubscribeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            IntervalMs = 0,
+        };
+        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => cmd.ValidateForTest());
+        ex.Message.ShouldContain("--interval-ms");
+    }
+
+    [Fact]
+    public void SubscribeCommand_validate_rejects_negative_interval()
+    {
+        var cmd = new SubscribeCommand
+        {
+            AmsNetId = "127.0.0.1.1.1",
+            SymbolPath = "MAIN.x",
+            IntervalMs = -100,
+        };
+        Should.Throw<CliFx.Exceptions.CommandException>(() => cmd.ValidateForTest());
+    }
+
+    // ---- Driver.TwinCAT.Cli-004 (PollOnly off BrowseCommand surface) ----
+
+    [Fact]
+    public void BrowseCommand_does_not_expose_poll_only_flag()
+    {
+        // Driver.TwinCAT.Cli-004: the flag has no observable effect on browse — surfacing it
+        // misleads users. After the refactor, PollOnly lives on an intermediate base shared
+        // only by the commands that actually consume native ADS notifications.
+        var props = typeof(BrowseCommand)
+            .GetProperties(BindingFlags.Public | BindingFlags.Instance);
+        props.ShouldNotContain(p => p.Name == "PollOnly");
+    }
+
+    [Fact]
+    public void ProbeCommand_still_exposes_poll_only_flag()
+    {
+        // Probe / Read / Write / Subscribe all build TwinCATDriverOptions and so still take
+        // the --poll-only toggle.
+        var props = typeof(ProbeCommand)
+            .GetProperties(BindingFlags.Public | BindingFlags.Instance);
+        props.ShouldContain(p => p.Name == "PollOnly");
+    }
+
+    // ---- Driver.TwinCAT.Cli-005 (probe --type short alias) ----
+
+    [Fact]
+    public void ProbeCommand_type_option_carries_short_alias_t()
+    {
+        // Driver.TwinCAT.Cli-005: --type on read/write/subscribe takes the -t short alias;
+        // probe must match so muscle memory works the same way across all four verbs.
+        var dataTypeProp = typeof(ProbeCommand).GetProperty("DataType");
+        dataTypeProp.ShouldNotBeNull();
+        var attr = dataTypeProp!.GetCustomAttribute<CommandOptionAttribute>();
+        attr.ShouldNotBeNull();
+        attr!.ShortName.ShouldBe('t');
+    }
+}
Author	SHA1	Message	Date
Joseph Doherty	2a941b255f	docs(code-reviews): regenerate index — 29 Low findings resolved Batch 5 cleared Open findings in Driver.AbCip.Cli, Driver.AbLegacy.Cli, Driver.S7.Cli, Driver.TwinCAT.Cli, and Driver.Modbus.Cli. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 08:35:12 -04:00
Joseph Doherty	80ef8806e0	fix(driver-modbus-cli): resolve Low code-review findings (Driver.Modbus.Cli-003,004,005,006,007,008) - Driver.Modbus.Cli-003: ModbusCommandBase.ValidateEndpoint rejects --port outside 1..65535, non-positive --timeout-ms, and --unit-id outside 1..247. - Driver.Modbus.Cli-004: wrapped SubscribeCommand's OnDataChange handler body in a try/catch (warn-and-swallow) and serialised the console write through a lock. - Driver.Modbus.Cli-005: Probe / Read / Write now catch the cancellation-during-init OperationCanceledException and print 'Cancelled.' instead of dumping a stack trace. - Driver.Modbus.Cli-006: ProbeCommand.ComputeVerdict derives the headline from BOTH the driver state and the probe snapshot's OPC UA quality class so the headline can't disagree with the wire result. - Driver.Modbus.Cli-007: docs/Driver.Modbus.Cli.md carries an explicit 'CLI scope' callout — the address-string grammar is a DriverConfig JSON feature; the CLI takes the structured triple only. - Driver.Modbus.Cli-008: pinned BuildOptions, ValidateEndpoint, the region-validation guards, ComputeVerdict, and the cancellation-during- initialize paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 08:35:05 -04:00
Joseph Doherty	f2ee027145	fix(driver-twincat-cli): resolve Low code-review findings (Driver.TwinCAT.Cli-001,002,003,004,005,006,007) - Driver.TwinCAT.Cli-001: TwinCATCommandBase.Validate rejects non-positive TimeoutMs / IntervalMs and AmsPort outside 1..65535; ExecuteAsync calls it first. - Driver.TwinCAT.Cli-002: SubscribeCommand serialises every WriteLine through a writeLock to remove the notification-callback vs banner interleave risk. - Driver.TwinCAT.Cli-003: SubscribeCommand.DescribeMechanism derives the banner label from the returned ISubscriptionHandle.DiagnosticId so it can't disagree with what the driver actually did. - Driver.TwinCAT.Cli-004: introduced TwinCATTagCommandBase carrying --poll-only + BuildOptions; BrowseCommand stays on the slimmer TwinCATCommandBase so --poll-only no longer surfaces in browse --help. - Driver.TwinCAT.Cli-005: ProbeCommand --type now carries the 't' short alias to match the other commands. - Driver.TwinCAT.Cli-006: 35 new tests covering Gateway / AmsAddress parse / BuildOptions / PollOnly / browse-helpers / probe-alias / mechanism derivation. - Driver.TwinCAT.Cli-007: replaced the empty-init <inheritdoc/> with an explicit summary warning future maintainers about the no-op init. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 08:34:57 -04:00
Joseph Doherty	67ef6c4ebc	fix(driver-s7-cli): resolve Low code-review findings (Driver.S7.Cli-004,005,006,007) - 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>	2026-05-23 08:34:48 -04:00
Joseph Doherty	f46e126208	fix(driver-ablegacy-cli): resolve Low code-review findings (Driver.AbLegacy.Cli-002,003,004,005,006,007) - Driver.AbLegacy.Cli-002: WriteCommand.Value description lists the full true/false, 1/0, on/off, yes/no alias set. - Driver.AbLegacy.Cli-003: SubscribeCommand serialises every WriteLine via a per-execution consoleGate lock so the poll-thread OnDataChange handler can't interleave with the banner. - Driver.AbLegacy.Cli-004: dropped 'await using var driver' in favour of a plain 'var driver' + explicit await ShutdownAsync in finally; the driver is no longer shut down twice. - Driver.AbLegacy.Cli-005: SubscribeCommand.IntervalMs description carries the PollGroupEngine 250ms-floor caveat; docs/Driver.AbLegacy.Cli.md spells out the same. - Driver.AbLegacy.Cli-006: ProbeCommand --type now carries the short alias 't' to match the other commands. - Driver.AbLegacy.Cli-007: BuildOptionsTests cover the probe-disabled, device-shape, tag-passthrough, timeout-propagation, and empty-tag-list paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 08:34:32 -04:00
Joseph Doherty	759af8c1bb	fix(driver-abcip-cli): resolve Low code-review findings (Driver.AbCip.Cli-003,004,005,006,007,008) - Driver.AbCip.Cli-003: SubscribeCommand prints the 'Subscribed' banner BEFORE wiring OnDataChange so the main thread can't interleave its write with the poll-thread handler. - Driver.AbCip.Cli-004: AbCipCommandBase.Timeout and SubscribeCommand validate TimeoutMs / IntervalMs and throw CommandException on non-positive values. - Driver.AbCip.Cli-005: every command now calls FlushLogging() in its finally block. - Driver.AbCip.Cli-006: Timeout init throws NotSupportedException with a pointer at TimeoutMs instead of silently swallowing assignments. - Driver.AbCip.Cli-007: added AbCipCommandBaseTests covering BuildOptions shape, probe / controller-browse / alarm toggles, host address, family selection, tag list passthrough. - Driver.AbCip.Cli-008: rewrote the opening paragraph in docs/Driver.AbCip.Cli.md to credit the six-CLI roster with a pointer at docs/DriverClis.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 08:34:25 -04:00