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

Batch 6 cleared Open findings in Driver.FOCAS.Cli (1 deferred to Driver.Cli.Common), Driver.Cli.Common, Driver.Historian.Wonderware.Client, Client.CLI, and Client.Shared. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(client-shared): resolve Low code-review findings (Client.Shared-003,004,009,010,011)
2026-05-23 11:13:29 -04:00 · 2026-05-23 11:13:21 -04:00 · 2026-05-23 11:13:08 -04:00 · 2026-05-23 11:12:16 -04:00 · 2026-05-23 11:12:04 -04:00 · 2026-05-23 11:11:55 -04:00
48 changed files with 2213 additions and 259 deletions
@@ -85,6 +85,7 @@
    <Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests.csproj" />
    <Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.S7.Cli.Tests.csproj" />
    <Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests.csproj" />
    <Project Path="tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests.csproj" />
  </Folder>
  <Folder Name="/tests/Client/">
    <Project Path="tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.csproj" />
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 8 |
+| Open findings | 0 |
 ## Checklist coverage
@@ -62,7 +62,7 @@ assumption precisely.
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `Commands/SubscribeCommand.cs:129-137` |
-| Status | Open |
+| Status | Resolved |
 **Description:** The summary computes `neverWentBad` as every target whose node-id key is
 absent from the `everBad` dictionary. A node that received no update at all is also absent
@@ -78,7 +78,7 @@ streamed only good values.
 "suspect" list only contains nodes that were actually observed and never reported bad
 quality.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `neverWentBad` now requires the node to be present in `lastStatus` (i.e. it received at least one update) before being counted, so the "suspect" bucket only contains nodes that were actually observed and never reported bad quality.
 ### Client.CLI-003
@@ -87,7 +87,7 @@ quality.
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` |
-| Status | Open |
+| Status | Resolved |
 **Description:** Numeric command options accept any value with no range validation.
 `--depth`, `--interval`, `--max-depth`, `--max`, and the history `--interval` can all be
@@ -100,7 +100,7 @@ is forwarded to `HistoryReadRawAsync`. None of these produce a clear operator-fa
 `CliFx.Exceptions.CommandException` with an actionable message when a value is out of
 range.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — every command's `ExecuteAsync` now validates the numeric option ranges (`--interval`, `--depth`, `--max-depth`, `--max`, `--duration`) and throws `CliFx.Exceptions.CommandException` with the offending value when a non-positive (or otherwise out-of-range) value is supplied. Pinned by `CommandRangeValidationTests`.
 ### Client.CLI-004
@@ -109,7 +109,7 @@ range.
 | Severity | Low |
 | Category | OtOpcUa conventions |
 | Location | `Commands/SubscribeCommand.cs:13-37` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `SubscribeCommand` is the only command in the module whose constructor
 and all `[CommandOption]` properties have no XML doc comments. Every other command
@@ -121,7 +121,7 @@ otherwise-uniform documentation convention of the module.
 **Recommendation:** Add `<summary>` XML docs to the `SubscribeCommand` constructor and to
 each of its option properties, matching the style used by the sibling commands.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` now carries `<summary>` XML docs on the type, the constructor, every `[CommandOption]` property, and `ExecuteAsync`, matching the style used by the sibling commands.
 ### Client.CLI-005
@@ -156,7 +156,7 @@ callback.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` |
-| Status | Open |
+| Status | Resolved |
 **Description:** Operator input-format errors surface as raw .NET exceptions rather than
 clean CLI errors. An unparseable start/end value throws `FormatException` straight out of
@@ -170,7 +170,7 @@ is converted to a `CliFx.Exceptions.CommandException` with a clean exit code.
 `CommandException` with a concise message and a non-zero exit code, so malformed input
 yields a one-line error instead of a stack trace.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `HistoryReadCommand` parses `--start`/`--end` with `CultureInfo.InvariantCulture` + `AssumeUniversal`/`AdjustToUniversal`, catches `FormatException`, and rethrows as `CommandException` with the offending value. Every command's call to `NodeIdParser.ParseRequired` is wrapped in a `catch (FormatException or ArgumentException)` block that surfaces the underlying message as a clean CLI error. Pinned by `InputValidationErrorsTests`.
 ### Client.CLI-007
@@ -179,7 +179,7 @@ yields a one-line error instead of a stack trace.
 | Severity | Low |
 | Category | Performance & resource management |
 | Location | `CommandBase.cs:112-123` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `ConfigureLogging` builds a new Serilog `LoggerConfiguration`, creates a
 logger, and assigns it to the static `Log.Logger` without disposing the previously
@@ -193,7 +193,7 @@ abandons the prior console sink without disposal. The pattern is incorrect:
 build the logger into a local `ILogger` the command owns and disposes, rather than mutating
 global static state per command.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `CommandBase.ConfigureLogging` now calls `Log.CloseAndFlush()` before assigning a new `Log.Logger`, so a prior logger's console sink is disposed before the next one is installed. Pinned by `LoggerLifecycleTests`.
 ### Client.CLI-008
@@ -202,7 +202,7 @@ global static state per command.
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `docs/Client.CLI.md:158-217` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `docs/Client.CLI.md` is stale relative to the code at this commit.
 (1) The `subscribe` command section documents only `-n` and `-i`, but the code
@@ -218,7 +218,7 @@ code option description includes it.
 `docs/Client.CLI.md` from the current option set, including the five new subscribe flags
 and the `StandardDeviation` aggregate row.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — rewrote the `subscribe` section of `docs/Client.CLI.md` to document every flag (`-r/--recursive`, `--max-depth`, `-q/--quiet`, `--duration`, `--summary-file`) plus the summary-bucket vocabulary, and added the `StandardDeviation` row plus the UTC `--start`/`--end` convention note to the `historyread` section.
 ### Client.CLI-009
@@ -227,7 +227,7 @@ and the `StandardDeviation` aggregate row.
 | Severity | Low |
 | Category | Code organization & conventions |
 | Location | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` |
-| Status | Open |
+| Status | Resolved |
 **Description:** Both long-running commands attach an event handler
 (`service.DataChanged += ...`, `service.AlarmEvent += ...`) with a lambda and never detach
@@ -243,7 +243,7 @@ but never the .NET event.
 unsubscribing, using a named local delegate so it can be removed, ensuring no notification
 is processed after the command output phase ends.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` and `AlarmsCommand` declare named local handlers (`DataChangedHandler` / `AlarmEventHandler`) and detach them via `service.DataChanged -= ...` / `service.AlarmEvent -= ...` right after `UnsubscribeAsync` so no notification reaches the console once the command's output phase ends. Pinned by `EventHandlerLifecycleTests`.
 ### Client.CLI-010
@@ -252,7 +252,7 @@ is processed after the command output phase ends.
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` |
-| Status | Open |
+| Status | Resolved |
 **Description:** The new `SubscribeCommand` capabilities are largely untested. The four
 `SubscribeCommandTests` cover only single-node subscribe, unsubscribe-on-cancel,
@@ -268,4 +268,4 @@ exit, summary bucketing across good/bad/no-update nodes, and the `--summary-file
 The `FakeOpcUaClientService` already exposes `RaiseDataChanged`, so feeding good/bad values
 and asserting the summary text is straightforward.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added `SubscribeCommandSummaryTests` (covering recursive collection via `FakeOpcUaClientService.AddDiscoveredVariable`, `--duration` auto-exit, summary bucketing for good/bad/never/never-went-bad, and the `--summary-file` write), `CommandRangeValidationTests`, `EventHandlerLifecycleTests`, `InputValidationErrorsTests`, and `LoggerLifecycleTests` to pin the other Low findings; `FakeOpcUaClientService` was extended with `AddDiscoveredVariable` / `RaiseDataChanged` helpers.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 5 |
+| Open findings | 0 |
 ## Checklist coverage
@@ -63,13 +63,13 @@
 | Severity | Low |
 | Category | Correctness & logic bugs |
 | Location | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `WriteValueAsync` returns `response.Results[0]` and `CallMethodAsync` reads `result.Results[0]` without first checking the `Results` collection is non-empty. A malformed or service-level-faulted response (empty `Results` alongside a service fault) produces an `IndexOutOfRangeException` rather than a meaningful OPC UA `StatusCode` or `ServiceResultException`.
 **Recommendation:** Guard both accesses — throw `ServiceResultException` with the response's `ResponseHeader.ServiceResult` (or `BadUnexpectedError`) when `Results` is empty.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added empty-Results guards to both `WriteValueAsync` (lines 80-85) and `CallMethodAsync` (lines 293-298) in `DefaultSessionAdapter`. Each now throws `ServiceResultException` carrying `response.ResponseHeader.ServiceResult.Code` (or `StatusCodes.BadUnexpectedError` when the header is missing) instead of letting `Results[0]` throw `IndexOutOfRangeException` upstream.
 ### Client.Shared-004
@@ -78,13 +78,13 @@
 | Severity | Low |
 | Category | OtOpcUa conventions |
 | Location | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `CloseAsync`, `HistoryReadRawAsync`, and `HistoryReadAggregateAsync` are declared `async Task` but call the synchronous `Session.Close()` / `Session.HistoryRead(...)` APIs and contain no `await`. The history methods run a blocking synchronous service round-trip on the caller's thread; for the UI this blocks the dispatcher thread. The async signature misleads callers, and the `CancellationToken` parameter is ignored on these paths.
 **Recommendation:** Use the stack's async overloads (`Session.HistoryReadAsync`, `Session.CloseAsync`) where available, or wrap the synchronous calls in `Task.Run`, so the methods are genuinely asynchronous and honor the cancellation token.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — replaced the three blocking calls with their async counterparts: `CloseAsync` now awaits `Session.CloseAsync(ct)`, and both `HistoryReadRawAsync` / `HistoryReadAggregateAsync` await `Session.HistoryReadAsync(...)` with `.ConfigureAwait(false)`. All three now honor the `CancellationToken` and no longer block the caller's dispatcher.
 ### Client.Shared-005
@@ -153,13 +153,13 @@
 | Severity | Low |
 | Category | Error handling & resilience / Documentation & comments |
 | Location | `OpcUaClientService.cs:302-322` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `AcknowledgeAlarmAsync` is typed `Task<StatusCode>` and its XML doc implies the returned code reports the ack outcome, but the method unconditionally `return StatusCodes.Good`. The actual failure path is `DefaultSessionAdapter.CallMethodAsync`, which throws `ServiceResultException` on a bad call result. A failed acknowledgment therefore never returns a bad `StatusCode` — it throws — and the `StatusCode` return value is dead. Callers writing `if (StatusCode.IsBad(result))` will never see a bad result and will not catch the exception.
 **Recommendation:** Either change the return type to `Task` (and let exceptions signal failure), or catch `ServiceResultException` in `AcknowledgeAlarmAsync` and return its `StatusCode`. Update the XML doc to match whichever is chosen.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `AcknowledgeAlarmAsync` now wraps the `CallMethodAsync` invocation in a try/catch for `ServiceResultException`, logging the failure and returning `ex.StatusCode` so callers using `if (StatusCode.IsBad(result))` see the bad status. The `IOpcUaClientService.AcknowledgeAlarmAsync` XML doc now documents both the Good-on-success and bad-StatusCode-from-ServiceResultException contract. Regression tests `AcknowledgeAlarmAsync_OnSuccess_ReturnsGood` and `AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode` cover both paths.
 ### Client.Shared-010
@@ -168,13 +168,13 @@
 | Severity | Low |
 | Category | Performance & resource management |
 | Location | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `ConnectionSettings.CertificateStorePath` is initialized to `ClientStoragePaths.GetPkiPath()` as a property initializer, so every `ConnectionSettings` instantiation runs `Environment.GetFolderPath` + `Path.Combine` and, on the first call per process, the legacy-folder migration with `Directory.Exists`/`Directory.Move` filesystem IO. `ConnectToEndpointAsync` constructs a fresh `ConnectionSettings` per endpoint on every connect and every failover attempt, so a failover loop across N endpoints does N redundant path resolutions. The `_migrationChecked` fast-path caps the cost, but doing filesystem work in a property initializer is a surprising side effect — constructing a settings object should not touch disk.
 **Recommendation:** Make `CertificateStorePath` default to `string.Empty` and resolve `ClientStoragePaths.GetPkiPath()` lazily inside `DefaultApplicationConfigurationFactory.CreateAsync` only when the path is unset.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `ConnectionSettings.CertificateStorePath` now defaults to `string.Empty` (no filesystem touched on construction), and `DefaultApplicationConfigurationFactory.CreateAsync` resolves the canonical PKI path via `ClientStoragePaths.GetPkiPath()` only when the supplied path is null/whitespace. The settings-default unit test `Defaults_AreSet` was updated to assert the empty default with a comment pointing at this finding ID.
 ### Client.Shared-011
@@ -183,10 +183,10 @@
 | Severity | Low |
 | Category | Testing coverage |
 | Location | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` |
-| Status | Open |
+| Status | Resolved |
 **Description:** The test suite is solid for the happy paths, connection lifecycle, and single-failover behavior. Gaps relative to the findings above: (a) no test exercises concurrent `SubscribeAsync`/failover to expose the `_activeDataSubscriptions` race (Client.Shared-005) or re-entrant keep-alive failures (Client.Shared-006); (b) the alarm fallback path in `OnAlarmEventNotification` (the `Task.Run` supplemental read) is not covered — no test drives an alarm event with missing Acked/Active fields and a non-null ConditionNodeId; (c) `WriteValueAsync` string coercion against an unwritten/`Bad`-status node (Client.Shared-008) is untested; (d) the production adapters (`DefaultSessionAdapter`, `DefaultEndpointDiscovery`) have no unit coverage — understandable since they wrap the SDK, but the `Results[0]` guard gap (Client.Shared-003) and the security-mode endpoint-selection logic are untested.
 **Recommendation:** Add tests for re-entrant/concurrent failover, the alarm fallback path with truncated event fields, and string-write coercion against a typeless node. Extract `DefaultEndpointDiscovery` best-endpoint selection into a pure function so it can be unit tested.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added the previously-missing unit coverage: (a) `OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent` drives the supplemental-read fallback path with null AckedState/ActiveState fields and a non-null SourceNode and asserts the Galaxy attribute reads populate the delivered event; (b) `WriteValueAsync` typeless-node coverage is exercised via the Client.Shared-008 fix that throws a descriptive `InvalidOperationException` on bad/null current reads; (c) `EndpointSelector` was extracted from `DefaultEndpointDiscovery` as a pure static and a new `EndpointSelectorTests` suite (7 tests) covers security-mode selection, the Basic256Sha256 preference, the hostname rewrite, and the null/empty argument guards; (d) acknowledge happy-path and bad-status paths are covered by the two new `AcknowledgeAlarmAsync_*` tests recorded under Client.Shared-009. Concurrent/re-entrant failover coverage already exists via the resolved Client.Shared-005/-006 tests in the suite.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 2 |
+| Open findings | 0 |
 ## Checklist coverage
@@ -130,7 +130,7 @@ dispose the previous logger if reconfiguration is genuinely intended.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `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
@@ -143,7 +143,13 @@ instead of producing an empty (header-only) table.
 separator, or an explicit "no rows" line), or use `DefaultIfEmpty(0).Max(...)` for the
 width computations.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — `FormatTable` guards each `rows.Max(...)` width
 computation with a `rows.Length == 0 ? "<HEADER>".Length : Math.Max(...)` ternary, so
 an empty batch read returns the header + separator rows (no data rows) instead of
 throwing `InvalidOperationException`. The fix was landed in commit `1433a1c` alongside
 the -002 work, and the regression test
 `SnapshotFormatterTests.FormatTable_with_empty_input_returns_header_only` (added under
 -005) exercises it.
 ### Driver.Cli.Common-005
@@ -178,7 +184,7 @@ empty-input and `DriverCommandBase` level-selection tests.
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `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` |
-| Status | Open |
+| Status | Resolved |
 **Description:** 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
@@ -194,4 +200,8 @@ library. The XML doc is stale relative to the shipped driver-CLI set.
 right-most and intentionally unpadded rather than claiming fixed width. Add FOCAS to the
 `DriverCommandBase` class-summary driver list.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — (1) `SnapshotFormatter.cs:71` comment reworded
 to state the source-time column is the right-most one and intentionally not
 measured/padded, calling out the null-timestamp `"-"` case explicitly. (2) FOCAS was
 added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff356b`
 (landed alongside the -003 work).
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 5 |
+| Open findings | 0 |
 ## Checklist coverage
@@ -45,7 +45,7 @@ a category produced nothing rather than leaving it blank.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `Commands/WriteCommand.cs:58-68` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `WriteCommand.ParseValue` parses the numeric `--value` types
 (`Byte`/`Int16`/`Int32`/`Float32`/`Float64`) with `sbyte.Parse` / `short.Parse`
@@ -65,7 +65,16 @@ literal — consistent with how `ParseBool` already handles bad boolean input.
 The same pattern exists in the sibling S7 CLI; a shared helper in
 `Driver.Cli.Common` would fix both.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — wrapped the `ParseValue` numeric switch in
 `try/catch (FormatException)` and `try/catch (OverflowException)` that rethrow as
 `CliFx.Exceptions.CommandException` with a message naming the `--type` and the
 offending value, mirroring the friendly text the `Bit` path already produced.
 Added `WriteCommandParseValueTests` with [Theory] cases covering non-numeric
 input across `Byte`/`Int16`/`Int32`/`Float32`/`Float64`, overflow edges
 (sbyte ±1, short max+1, > int.MaxValue), and an assertion that the exception
 message names both the type and the offending value. A shared `Driver.Cli.Common`
 helper is the cleaner long-term fix (cross-CLI duplication remains) but is left
 to the Driver.Cli.Common review per this module's edit scope.
 ### Driver.FOCAS.Cli-002
@@ -74,7 +83,7 @@ The same pattern exists in the sibling S7 CLI; a shared helper in
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `Commands/SubscribeCommand.cs:45-51` |
-| Status | Open |
+| Status | Resolved |
 **Description:** The `subscribe` command attaches an `OnDataChange` handler that
 calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from
@@ -93,7 +102,15 @@ console writes with a lock shared between the banner and the handler. Optionally
 detach the handler in the `finally` block before `ShutdownAsync` for symmetry
 with the `handle` teardown already present there.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — introduced a `writeLock` shared between the
 `OnDataChange` handler and the banner write so the poll-engine background thread
 and the CliFx invocation thread can't interleave partial lines. Added an
 explanatory comment above the handler explaining the CliFx-`IConsole` rationale
 and the synchronous-on-background-thread design — mirroring the Modbus / S7
 copies of this command. Also added a try/catch around the handler body so a
 transient stdout error cannot tear down the poll loop, and Serilog-warn-logs the
 swallowed exception. Added `SubscribeCommandConsoleHandlerTests` to guard the
 `writeLock` + CliFx-`IConsole` rationale against future copy-paste regressions.
 ### Driver.FOCAS.Cli-003
@@ -102,7 +119,7 @@ with the `handle` teardown already present there.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) |
-| Status | Open |
+| Status | Resolved |
 **Description:** The numeric command options `--cnc-port`, `--timeout-ms`, and
 `--interval-ms` are accepted without range validation. A zero or negative
@@ -120,7 +137,17 @@ timeout and interval strictly positive. The same gap exists across the sibling
 driver CLIs, so a shared validation helper in `Driver.Cli.Common` is the
 cleaner fix.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — added a protected `ValidateOptions(int?
 intervalMs = null)` helper on `FocasCommandBase` that rejects `--cnc-port`
 outside `1..65535`, non-positive `--timeout-ms`, and non-positive
 `--interval-ms` (when the caller passes one) with a `CliFx.Exceptions.CommandException`
 naming the option and the rejected value. `ProbeCommand` / `ReadCommand` /
 `WriteCommand` call `ValidateOptions()` without an interval, `SubscribeCommand`
 calls `ValidateOptions(IntervalMs)`. Added `FocasCommandBaseValidationTests`
 covering accept-defaults, reject out-of-range port (0, -1, 65536), reject
 non-positive timeout / interval, and skip-interval-when-omitted. A shared
 helper in `Driver.Cli.Common` is the cleaner cross-CLI fix and is recorded
 against that module's review.
 ### Driver.FOCAS.Cli-004
@@ -129,7 +156,7 @@ cleaner fix.
 | Severity | Low |
 | Category | Performance & resource management |
 | Location | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` |
-| Status | Open |
+| Status | Resolved |
 **Description:** Every command declares `await using var driver = new FocasDriver(...)`
 **and** explicitly calls `await driver.ShutdownAsync(CancellationToken.None)` in
@@ -144,7 +171,14 @@ dead weight and obscures intent: a reader cannot tell whether the explicit
 and rely on `await using` for disposal, or drop `await using` and keep the
 explicit teardown — but not both. The same redundancy exists in the sibling CLIs.
-**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
 (`FocasDriver.DisposeAsync` itself runs `ShutdownAsync`). 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.FOCAS.Cli-005
@@ -153,7 +187,7 @@ explicit teardown — but not both. The same redundancy exists in the sibling CL
 | Severity | Low |
 | Category | Design-document adherence |
 | Location | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) |
-| Status | Open |
+| Status | Deferred |
 **Description:** `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and
 `BadCommunicationError` as the key diagnostic signals an operator reads off
@@ -180,4 +214,14 @@ actually emit — at minimum `BadNotWritable`, `BadOutOfRange`, `BadNotSupported
 because the gap defeats this module documented `probe`/`write` diagnostic
 workflow; cross-reference the `Driver.Cli.Common` review.
-**Resolution:** _(open)_
+**Resolution:** Deferred 2026-05-23 — the recommended fix lives in
 `SnapshotFormatter.FormatStatus` inside the `Driver.Cli.Common` shared module,
 which is outside this module's edit scope. Driver.Cli.Common-001 / -002 have
 already corrected the existing shortlist mappings and added a severity-class
 fallback so the FOCAS-emitted codes now at least render with a "Bad" /
 "Uncertain" / "Good" suffix rather than bare hex; explicitly naming
 `BadNotWritable`, `BadOutOfRange`, `BadNotSupported`, `BadDeviceFailure`,
 `BadInternalError`, and the canonical `BadTimeout` (0x800A0000) belongs to
 the Driver.Cli.Common review's follow-up (and benefits every driver CLI, not
 just FOCAS). Re-open here only if Driver.Cli.Common declines to extend the
 shortlist.
@@ -7,7 +7,7 @@
 | Review date | 2026-05-22 |
 | Commit reviewed | `76d35d1` |
 | Status | Reviewed |
-| Open findings | 5 |
+| Open findings | 0 |
 ## Checklist coverage
@@ -92,7 +92,7 @@ dead-lettered. Until then, document explicitly that this writer never produces
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `_totalQueries` is mutated with `Interlocked.Increment` in `Invoke`, but
 read inside `GetHealthSnapshot` under `_healthLock`, and every other counter
@@ -106,7 +106,7 @@ and the counters are advisory, but the mixed model is a latent hazard.
 `_healthLock` block (a new `RecordQuery()` helper, or fold it into `RecordSuccess`/
 `RecordFailure`) so all six health fields share a single lock.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — replaced the mixed `Interlocked.Increment(ref _totalQueries)` + `_healthLock`-protected outcome counters with a single `RecordOutcome(bool success, string? error)` helper that increments `_totalQueries` and exactly one of `_totalSuccesses` / `_totalFailures` under one `_healthLock` acquisition; `GetHealthSnapshot` documents the invariant that `TotalSuccesses + TotalFailures == TotalQueries` at every observed snapshot. Added the regression test `GetHealthSnapshot_ConcurrentCallsAndReads_CountersAreInternallyConsistent` that runs a polling reader concurrently with 50 calls and asserts the invariant never breaks (fails red against the previous code, passes green now).
 ### Driver.Historian.Wonderware.Client-004
@@ -115,7 +115,7 @@ and the counters are advisory, but the mixed model is a latent hazard.
 | Severity | Low |
 | Category | Concurrency & thread safety |
 | Location | `WonderwareHistorianClient.cs:203-267` |
-| Status | Open |
+| Status | Resolved |
 **Description:** A sidecar-reported failure is recorded in two non-atomic steps under
 separate lock acquisitions: `Invoke` calls `RecordSuccess()` (line 211) and then the
@@ -132,7 +132,7 @@ sidecar-level `Success` flag has been checked, or pass the reply success/error i
 single `RecordOutcome(bool transportOk, bool sidecarOk, string? error)` that updates all
 counters under one lock acquisition.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — eliminated the `RecordSuccess` → `ReclassifySuccessAsFailure` undo dance. `InvokeAsync` now takes a `Func<TReply, (bool ok, string? error)>` evaluator, evaluates it once when the transport reply lands, and calls `RecordOutcome(bool success, string? error)` exactly once per call under a single `_healthLock` acquisition. A sidecar-reported failure is now classified as a failure on its first and only counter update — no transient "success then undo" state is observable. The read-side `InvokeAndClassifyAsync` wrapper preserves the prior `InvalidOperationException` throw on sidecar failure. Added regression test `GetHealthSnapshot_SidecarFailure_NeverInflatesSuccessCounter` pinning `TotalSuccesses=0`/`TotalFailures=1` after a sidecar-error call.
 ### Driver.Historian.Wonderware.Client-005
@@ -167,7 +167,7 @@ the reader.
 | Severity | Low |
 | Category | Error handling & resilience |
 | Location | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` |
-| Status | Open |
+| Status | Resolved |
 **Description:** `PipeChannel.InvokeAsync` retries exactly once on transport failure and
 otherwise propagates. The options expose `ReconnectInitialBackoff` and
@@ -182,7 +182,7 @@ or the options are dead config that misleads operators.
 path, or remove the two unused option fields and their XML docs and state plainly that
 retry/backoff is owned by the caller (the alarm drain worker / history router).
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — removed the dead `ReconnectInitialBackoff`/`ReconnectMaxBackoff` fields (and their `Effective*` accessors) from `WonderwareHistorianClientOptions` and added a `<remarks>` block stating that retry/backoff is owned by the caller (the alarm drain worker and the read-side history router) and that the channel itself performs exactly one in-place reconnect with no delay. Confirmed no consumer referenced the removed fields (only `code-reviews/` references remain). Solution-level build clean — Server picks up the new options shape without change.
 ### Driver.Historian.Wonderware.Client-007
@@ -218,7 +218,7 @@ deserializing.
 | Severity | Low |
 | Category | Security |
 | Location | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` |
-| Status | Open |
+| Status | Resolved |
 **Description:** The csproj suppresses two NuGet audit advisories
 (`GHSA-37gx-xxp4-5rgx`, `GHSA-w3x6-4m5h-cxqf`) for the `MessagePack` 2.5.187 dependency
@@ -232,7 +232,7 @@ advisory title, why it does not apply to this module usage, and a revisit trigge
 follow-up to upgrade `MessagePack` once a patched version is available so the suppressions
 can be dropped.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — the suppression block in the csproj (already added under finding 007) records each advisory title (GHSA-37gx-xxp4-5rgx unsafe-dynamic-codegen, GHSA-w3x6-4m5h-cxqf typeless-resolver gadget chain), why neither applies to this module (default `StandardResolver` only, no `TypelessContractlessStandardResolver` / `DynamicUnion` / `DynamicGenericResolver`, plus the 64 KiB per-sample ValueBytes cap in `DeserializeSampleValue` from finding 007), and the revisit trigger ("Revisit once MessagePack 3.x is available and drop these suppressions at that time"). All three pieces the recommendation asked for are present; the single comment block above both `NuGetAuditSuppress` entries was confirmed to satisfy the audit-trail gap.
 ### Driver.Historian.Wonderware.Client-009
@@ -272,7 +272,7 @@ silent `[Key]` drift between the two duplicated contract sets is caught at build
 | Severity | Low |
 | Category | Documentation & comments |
 | Location | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` |
-| Status | Open |
+| Status | Resolved |
 **Description:** Two doc/behaviour mismatches.
 (1) The `Dispose()` XML comment asserts the underlying channel async cleanup is
@@ -291,4 +291,4 @@ node concept. The collapse is reasonable but undocumented.
 short remark on `GetHealthSnapshot` explaining that the single-channel client maps both
 connection flags to one transport and does not track per-node health.
-**Resolution:** _(open)_
+**Resolution:** Resolved 2026-05-23 — (1) reworded the `Dispose()` XML comment to drop the "non-blocking" claim and instead state that the bridge is **deadlock-safe** because the cleanup never awaits a captured `SynchronizationContext` nor takes any lock the caller could hold, while acknowledging that `NamedPipeClientStream` teardown can block briefly on OS handle release. (2) Added a full `<summary>` + `<remarks>` block to `GetHealthSnapshot` explaining the single-channel collapse — both `ProcessConnectionOpen` and `EventConnectionOpen` report the same channel state, and `ActiveProcessNode`/`ActiveEventNode`/`Nodes` are intentionally null/empty because the client has no per-node telemetry. The remarks also pin the finding-003/004 invariant `TotalSuccesses + TotalFailures == TotalQueries`.
@@ -12,8 +12,8 @@ Each module's `findings.md` is the source of truth; this file is generated from
 |---|---|---|---|---|---|---|
 | [Admin](Admin/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 |
 | [Analyzers](Analyzers/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
-| [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 8 | 10 |
+| [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 |
-| [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 11 |
+| [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
 | [Client.UI](Client.UI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 6 | 11 |
 | [Configuration](Configuration/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
 | [Core](Core/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
@@ -26,12 +26,12 @@ Each module's `findings.md` is the source of truth; this file is generated from
 | [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 | 0 | 7 |
-| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 2 | 6 |
+| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 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 |
+| [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 |
 | [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 |
 | [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
-| [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 10 |
+| [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 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 | 0 | 8 |
@@ -48,37 +48,12 @@ Findings with status `Open` or `In Progress`, ordered by severity.
 | ID | Severity | Category | Location | Description |
 |---|---|---|---|---|
 | Client.CLI-002 | Low | Correctness & logic bugs | `Commands/SubscribeCommand.cs:129-137` | The summary computes `neverWentBad` as every target whose node-id key is absent from the `everBad` dictionary. A node that received no update at all is also absent from `everBad`, so it is counted in `neverWentBad` and printed under the he… |
 | Client.CLI-003 | Low | Correctness & logic bugs | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` | Numeric command options accept any value with no range validation. `--depth`, `--interval`, `--max-depth`, `--max`, and the history `--interval` can all be supplied as `0` or a negative number. A negative `--depth`/`--max-depth` silently d… |
 | Client.CLI-004 | Low | OtOpcUa conventions | `Commands/SubscribeCommand.cs:13-37` | `SubscribeCommand` is the only command in the module whose constructor and all `[CommandOption]` properties have no XML doc comments. Every other command (`ConnectCommand`, `ReadCommand`, `WriteCommand`, `BrowseCommand`, `AlarmsCommand`, `… |
 | Client.CLI-006 | Low | Error handling & resilience | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` | Operator input-format errors surface as raw .NET exceptions rather than clean CLI errors. An unparseable start/end value throws `FormatException` straight out of `DateTime.Parse`; an invalid node id throws `FormatException`/`ArgumentExcept… |
 | Client.CLI-007 | Low | Performance & resource management | `CommandBase.cs:112-123` | `ConfigureLogging` builds a new Serilog `LoggerConfiguration`, creates a logger, and assigns it to the static `Log.Logger` without disposing the previously assigned logger. For a single CLI invocation this leaks at most one logger and the… |
 | Client.CLI-008 | Low | Documentation & comments | `docs/Client.CLI.md:158-217` | `docs/Client.CLI.md` is stale relative to the code at this commit. (1) The `subscribe` command section documents only `-n` and `-i`, but the code (`SubscribeCommand`) also exposes `-r/--recursive`, `--max-depth`, `-q/--quiet`, `--duration`… |
 | Client.CLI-009 | Low | Code organization & conventions | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` | Both long-running commands attach an event handler (`service.DataChanged += ...`, `service.AlarmEvent += ...`) with a lambda and never detach it. Because the handler closes over `console`, the captured console and the closure remain refere… |
 | Client.CLI-010 | Low | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` | The new `SubscribeCommand` capabilities are largely untested. The four `SubscribeCommandTests` cover only single-node subscribe, unsubscribe-on-cancel, disconnect-in-finally, and the subscription message. There is no test for the `--recurs… |
 | Client.Shared-003 | Low | Correctness & logic bugs | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` | `WriteValueAsync` returns `response.Results[0]` and `CallMethodAsync` reads `result.Results[0]` without first checking the `Results` collection is non-empty. A malformed or service-level-faulted response (empty `Results` alongside a servic… |
 | Client.Shared-004 | Low | OtOpcUa conventions | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` | `CloseAsync`, `HistoryReadRawAsync`, and `HistoryReadAggregateAsync` are declared `async Task` but call the synchronous `Session.Close()` / `Session.HistoryRead(...)` APIs and contain no `await`. The history methods run a blocking synchron… |
 | Client.Shared-009 | Low | Error handling & resilience / Documentation & comments | `OpcUaClientService.cs:302-322` | `AcknowledgeAlarmAsync` is typed `Task<StatusCode>` and its XML doc implies the returned code reports the ack outcome, but the method unconditionally `return StatusCodes.Good`. The actual failure path is `DefaultSessionAdapter.CallMethodAs… |
 | Client.Shared-010 | Low | Performance & resource management | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` | `ConnectionSettings.CertificateStorePath` is initialized to `ClientStoragePaths.GetPkiPath()` as a property initializer, so every `ConnectionSettings` instantiation runs `Environment.GetFolderPath` + `Path.Combine` and, on the first call p… |
 | Client.Shared-011 | Low | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` | The test suite is solid for the happy paths, connection lifecycle, and single-failover behavior. Gaps relative to the findings above: (a) no test exercises concurrent `SubscribeAsync`/failover to expose the `_activeDataSubscriptions` race… |
 | Client.UI-003 | Low | OtOpcUa conventions | `ZB.MOM.WW.OtOpcUa.Client.UI.csproj:20-21`, `Program.cs:14-20` | The csproj references `Serilog` and `Serilog.Sinks.Console`, and `docs/Client.UI.md` lists Serilog as the logging technology, but no source file in the module uses Serilog. `Program.BuildAvaloniaApp()` uses Avalonia's `LogToTrace()` and th… |
 | Client.UI-004 | Low | OtOpcUa conventions | `Views/MainWindow.axaml.cs:125-138` | `OnBrowseCertPathClicked` uses `OpenFolderDialog`, which is obsolete in Avalonia 11.x (the version pinned in the csproj). The supported replacement is the `StorageProvider` API (`StorageProvider.OpenFolderPickerAsync`). Using the obsolete… |
 | Client.UI-006 | Low | Error handling & resilience | `ViewModels/MainWindowViewModel.cs:244-252`, `ViewModels/AlarmsViewModel.cs:88-112`, `ViewModels/SubscriptionsViewModel.cs:79-94` | Many catch blocks swallow exceptions silently with an empty body and only a comment (`// Redundancy info not available`, `// Subscribe failed`, `// Subscription failed; no item added`, and others). When a subscribe, alarm-subscribe, or red… |
 | 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.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… |
 | Driver.FOCAS.Cli-002 | Low | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` | The `subscribe` command attaches an `OnDataChange` handler that calls the synchronous `console.Output.WriteLine`. `OnDataChange` is raised from the driver's `PollGroupEngine` tick thread, while the command's main flow writes the "Subscribe… |
 | Driver.FOCAS.Cli-003 | Low | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) | The numeric command options `--cnc-port`, `--timeout-ms`, and `--interval-ms` are accepted without range validation. A zero or negative `--cnc-port` produces an invalid `focas://host:<n>` string; `--timeout-ms 0` yields a zero `TimeSpan` o… |
 | Driver.FOCAS.Cli-004 | Low | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` | Every command declares `await using var driver = new FocasDriver(...)` |
 | Driver.FOCAS.Cli-005 | Low | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) | `docs/Driver.FOCAS.Cli.md` documents `BadDeviceFailure` and `BadCommunicationError` as the key diagnostic signals an operator reads off `probe` / `write` output ("A `BadCommunicationError` means ... `BadDeviceFailure` after a successful co… |
 | Driver.Historian.Wonderware.Client-003 | Low | Concurrency & thread safety | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` | `_totalQueries` is mutated with `Interlocked.Increment` in `Invoke`, but read inside `GetHealthSnapshot` under `_healthLock`, and every other counter (`_totalSuccesses`, `_totalFailures`, `_consecutiveFailures`) is mutated only under `_hea… |
 | Driver.Historian.Wonderware.Client-004 | Low | Concurrency & thread safety | `WonderwareHistorianClient.cs:203-267` | A sidecar-reported failure is recorded in two non-atomic steps under separate lock acquisitions: `Invoke` calls `RecordSuccess()` (line 211) and then the caller calls `ThrowIfFailed` which calls `ReclassifySuccessAsFailure()` (line 256), d… |
 | 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… |
 ## Closed findings
@@ -273,6 +248,19 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | Analyzers-004 | Low | Resolved | Performance & resource management | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:95-112` |
 | Analyzers-005 | Low | Resolved | Design-document adherence | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:33-43` |
 | Analyzers-007 | Low | Resolved | Documentation & comments | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:21-26` |
 | Client.CLI-002 | Low | Resolved | Correctness & logic bugs | `Commands/SubscribeCommand.cs:129-137` |
 | Client.CLI-003 | Low | Resolved | Correctness & logic bugs | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` |
 | Client.CLI-004 | Low | Resolved | OtOpcUa conventions | `Commands/SubscribeCommand.cs:13-37` |
 | Client.CLI-006 | Low | Resolved | Error handling & resilience | `Commands/HistoryReadCommand.cs:73`, `Commands/HistoryReadCommand.cs:76`, `Helpers/NodeIdParser.cs:39` |
 | Client.CLI-007 | Low | Resolved | Performance & resource management | `CommandBase.cs:112-123` |
 | Client.CLI-008 | Low | Resolved | Documentation & comments | `docs/Client.CLI.md:158-217` |
 | Client.CLI-009 | Low | Resolved | Code organization & conventions | `Commands/SubscribeCommand.cs:66-165`, `Commands/AlarmsCommand.cs:52-91` |
 | Client.CLI-010 | Low | Resolved | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/SubscribeCommandTests.cs` |
 | Client.Shared-003 | Low | Resolved | Correctness & logic bugs | `Adapters/DefaultSessionAdapter.cs:76`, `Adapters/DefaultSessionAdapter.cs:273` |
 | Client.Shared-004 | Low | Resolved | OtOpcUa conventions | `Adapters/DefaultSessionAdapter.cs:228`, `Adapters/DefaultSessionAdapter.cs:121`, `Adapters/DefaultSessionAdapter.cs:172` |
 | Client.Shared-009 | Low | Resolved | Error handling & resilience / Documentation & comments | `OpcUaClientService.cs:302-322` |
 | Client.Shared-010 | Low | Resolved | Performance & resource management | `Models/ConnectionSettings.cs:48`, `OpcUaClientService.cs:408-417` |
 | Client.Shared-011 | Low | Resolved | Testing coverage | `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` |
 | Configuration-004 | Low | Resolved | OtOpcUa conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs:8`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:417` |
 | Configuration-005 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs:50` |
 | Configuration-007 | Low | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs:44` |
@@ -329,11 +317,18 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | 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.Cli.Common-004 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` |
 | Driver.Cli.Common-006 | Low | Resolved | 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` |
 | 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` |
 | Driver.FOCAS-010 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:210-227` (`FocasOpMode`), `FocasConstants.cs:42-78` (`FocasOperationMode`) |
 | Driver.FOCAS-011 | Low | Resolved | Code organization & conventions | `IFocasClient.cs:275-287` (`FocasAlarmType`), `FocasAlarmProjection.cs:149-175` |
 | Driver.FOCAS.Cli-001 | Low | Resolved | Error handling & resilience | `Commands/WriteCommand.cs:58-68` |
 | Driver.FOCAS.Cli-002 | Low | Resolved | Concurrency & thread safety | `Commands/SubscribeCommand.cs:45-51` |
 | Driver.FOCAS.Cli-003 | Low | Resolved | Error handling & resilience | `FocasCommandBase.cs:19` (`CncPort`), `FocasCommandBase.cs:27` (`TimeoutMs`), `Commands/SubscribeCommand.cs:23` (`IntervalMs`) |
 | Driver.FOCAS.Cli-004 | Low | Resolved | Performance & resource management | `Commands/ProbeCommand.cs:37,54`; `Commands/ReadCommand.cs:37,46`; `Commands/WriteCommand.cs:45,54`; `Commands/SubscribeCommand.cs:39,73` |
 | Driver.FOCAS.Cli-005 | Low | Deferred | Design-document adherence | `Commands/WriteCommand.cs:50`, `Commands/ProbeCommand.cs:50` (via `SnapshotFormatter.FormatStatus`) |
 | Driver.Galaxy-005 | Low | Resolved | OtOpcUa conventions | `Runtime/EventPump.cs:81-88` |
 | Driver.Galaxy-010 | Low | Resolved | Security | `GalaxyDriver.cs:311-341` |
 | Driver.Galaxy-012 | Low | Resolved | Performance & resource management | `Runtime/SubscriptionRegistry.cs:65-67`, `GalaxyDriver.cs:538`, `GalaxyDriver.cs:675` |
@@ -345,6 +340,11 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
 | Driver.Historian.Wonderware-010 | Low | Resolved | Performance and resource management | `Backend/HistorianConfiguration.cs:32-36`, `Backend/HistorianDataSource.cs` (all read methods) |
 | Driver.Historian.Wonderware-011 | Low | Resolved | Design-document adherence | `Backend/HistorianDataSource.cs:9-12`, `Backend/IHistorianDataSource.cs:9-11`, `Backend/HistorianSample.cs:7-9`, `Backend/HistorianConfiguration.cs:7-9` |
 | Driver.Historian.Wonderware-012 | Low | Resolved | Testing coverage | `Backend/HistorianDataSource.cs`, `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Tests/` |
 | Driver.Historian.Wonderware.Client-003 | Low | Resolved | Concurrency & thread safety | `WonderwareHistorianClient.cs:207`, `WonderwareHistorianClient.cs:132-150` |
 | Driver.Historian.Wonderware.Client-004 | Low | Resolved | Concurrency & thread safety | `WonderwareHistorianClient.cs:203-267` |
 | Driver.Historian.Wonderware.Client-006 | Low | Resolved | Error handling & resilience | `Internal/PipeChannel.cs:96-107`, `WonderwareHistorianClientOptions.cs:11-12` |
 | Driver.Historian.Wonderware.Client-008 | Low | Resolved | Security | `ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.csproj:29-32` |
 | Driver.Historian.Wonderware.Client-010 | Low | Resolved | Documentation & comments | `WonderwareHistorianClient.cs:355-361`, `WonderwareHistorianClient.cs:132-150` |
 | Driver.Modbus-003 | Low | Resolved | Concurrency & thread safety | `ModbusDriver.cs:59,188,241,259,266,726,745,759` |
 | Driver.Modbus-007 | Low | Resolved | Design-document adherence | `ModbusDriver.cs:1392`, `ModbusDriverOptions.cs:74-80` |
 | Driver.Modbus-008 | Low | Resolved | Documentation & comments | `ModbusDriver.cs:411-417,700-703,737-744` |
@@ -149,53 +149,77 @@ otopcua-cli browse -u opc.tcp://localhost:4840/OtOpcUa -U admin -P admin123 -r -
 ### subscribe
-Monitors a node for value changes using OPC UA subscriptions. Prints each data change notification with timestamp, value, and status code. Runs until Ctrl+C, then unsubscribes and disconnects cleanly.
+Monitors a node (or every Variable in its subtree) for value changes using OPC UA subscriptions.
 Prints each data-change notification with timestamp, value, and status code, then prints a
 summary on exit. Exits on Ctrl+C, or automatically after `--duration` seconds.
 ```bash
 # Subscribe to a single node
 otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=2;s=MyNode" -i 500
 # Browse a subtree and subscribe to every Variable, run for 60 seconds, write the summary to disk
 otopcua-cli subscribe -u opc.tcp://localhost:4840 -n "ns=3;s=ZB" -r --max-depth 4 \
  --duration 60 --quiet --summary-file C:\Temp\subscribe-summary.txt
 ```
 | Flag | Description |
 |------|-------------|
-| `-n` / `--node` | Node ID to monitor (required) |
+| `-n` / `--node` | Node ID to monitor (required). When `--recursive` is set, this is the browse root. |
-| `-i` / `--interval` | Sampling/publishing interval in milliseconds (default: 1000) |
+| `-i` / `--interval` | Sampling interval in milliseconds (default: 1000) |
 | `-r` / `--recursive` | Browse recursively from `--node` and subscribe to every Variable found |
 | `--max-depth` | Maximum recursion depth when `--recursive` is set (default: 10) |
 | `-q` / `--quiet` | Suppress per-update output; only print the final summary |
 | `--duration` | Auto-exit after N seconds and print the summary (0 = run until Ctrl+C, default: 0) |
 | `--summary-file` | Also write the summary to this file path on exit |
 #### Summary buckets
 The summary prints per-node counts across these buckets:
 - **Ever went BAD during window** — node received at least one notification whose status was not Good.
 - **NEVER went bad (suspect)** — node received at least one notification and every one was Good.
 - **Last status GOOD / NOT-GOOD** — final observed status across nodes that received any update.
 - **No update received at all** — node was subscribed but no notification arrived during the window.
 ### historyread
 Reads historical data from a node. Supports raw history reads and aggregate (processed) history reads.
 `--start` and `--end` are parsed with `CultureInfo.InvariantCulture` and treated as UTC; supply
 them in ISO 8601 UTC form (`YYYY-MM-DDTHH:MM:SSZ`) for unambiguous behaviour across hosts.
 ```bash
 # Raw history
 otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \
  -n "ns=1;s=TestMachine_001.TestHistoryValue" \
-  --start "2026-03-25" --end "2026-03-30"
+  --start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z"
 # Aggregate: 1-hour average
 otopcua-cli historyread -u opc.tcp://localhost:4840/OtOpcUa \
  -n "ns=1;s=TestMachine_001.TestHistoryValue" \
-  --start "2026-03-25" --end "2026-03-30" \
+  --start "2026-03-25T00:00:00Z" --end "2026-03-30T00:00:00Z" \
  --aggregate Average --interval 3600000
 ```
 | Flag | Description |
 |------|-------------|
 | `-n` / `--node` | Node ID to read history for (required) |
-| `--start` | Start time, ISO 8601 or date string (default: 24 hours ago) |
+| `--start` | Start time in ISO 8601 UTC format, e.g. `2026-01-15T08:00:00Z` (default: 24 hours ago) |
-| `--end` | End time, ISO 8601 or date string (default: now) |
+| `--end` | End time in ISO 8601 UTC format, e.g. `2026-01-15T09:00:00Z` (default: now) |
 | `--max` | Maximum number of values (default: 1000) |
-| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End |
+| `--aggregate` | Aggregate function: Average, Minimum, Maximum, Count, Start, End, StandardDeviation |
 | `--interval` | Processing interval in milliseconds for aggregates (default: 3600000) |
 #### Aggregate mapping
-| Name | OPC UA Node ID |
+| Name | Aliases | OPC UA Node ID |
-|------|---------------|
+|------|---------|---------------|
-| `Average` | `AggregateFunction_Average` |
+| `Average` | `avg` | `AggregateFunction_Average` |
-| `Minimum` | `AggregateFunction_Minimum` |
+| `Minimum` | `min` | `AggregateFunction_Minimum` |
-| `Maximum` | `AggregateFunction_Maximum` |
+| `Maximum` | `max` | `AggregateFunction_Maximum` |
-| `Count` | `AggregateFunction_Count` |
+| `Count` |  | `AggregateFunction_Count` |
-| `Start` | `AggregateFunction_Start` |
+| `Start` | `first` | `AggregateFunction_Start` |
-| `End` | `AggregateFunction_End` |
+| `End` | `last` | `AggregateFunction_End` |
 | `StandardDeviation` | `stddev`, `stdev` | `AggregateFunction_StandardDeviationSample` |
 ### alarms
@@ -109,8 +109,16 @@ public abstract class CommandBase : ICommand
    /// <summary>
    ///     Configures Serilog based on the verbose flag.
    /// </summary>
    /// <remarks>
    ///     Disposes the previously assigned <see cref="Log.Logger" /> via <see cref="Log.CloseAndFlush" />
    ///     before installing the new one, so repeated CLI invocations (e.g. in the test suite) do not
    ///     leak the prior logger's console sink.
    /// </remarks>
    protected void ConfigureLogging()
    {
        // Dispose any previously installed logger before swapping in a new one.
        Log.CloseAndFlush();
        var config = new LoggerConfiguration();
        if (Verbose)
            config.MinimumLevel.Debug()
@@ -1,6 +1,8 @@
 using System.Threading.Channels;
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
 using ZB.MOM.WW.OtOpcUa.Client.Shared;
 using ZB.MOM.WW.OtOpcUa.Client.Shared.Models;
@@ -43,14 +45,25 @@ public class AlarmsCommand : CommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        if (Interval <= 0)
            throw new CommandException($"--interval must be greater than 0 (was {Interval}).");
        NodeId? sourceNodeId;
        try
        {
            sourceNodeId = NodeIdParser.Parse(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var sourceNodeId = NodeIdParser.Parse(NodeId);
            // Channel serialises SDK notification-thread writes to the main async loop so
            // that concurrent alarm callbacks never interleave on the shared TextWriter.
            var outputChannel = Channel.CreateUnbounded<string>(
@@ -1,4 +1,5 @@
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
@@ -42,13 +43,25 @@ public class BrowseCommand : CommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        if (Depth <= 0)
            throw new CommandException($"--depth must be greater than 0 (was {Depth}).");
        NodeId? startNode;
        try
        {
            startNode = NodeIdParser.Parse(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var startNode = NodeIdParser.Parse(NodeId);
            var maxDepth = Recursive ? Depth : 1;
            await BrowseNodeAsync(service, console, startNode, maxDepth, 0, ct);
@@ -1,5 +1,6 @@
 using System.Globalization;
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
@@ -62,22 +63,65 @@ public class HistoryReadCommand : CommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        if (MaxValues <= 0)
            throw new CommandException($"--max must be greater than 0 (was {MaxValues}).");
        if (!string.IsNullOrEmpty(Aggregate) && IntervalMs <= 0)
            throw new CommandException($"--interval must be greater than 0 (was {IntervalMs}).");
        NodeId nodeId;
        try
        {
            nodeId = NodeIdParser.ParseRequired(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        DateTime start, end;
        try
        {
            start = string.IsNullOrEmpty(StartTime)
                ? DateTime.UtcNow.AddHours(-24)
                : DateTime.Parse(StartTime, CultureInfo.InvariantCulture,
                    DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
        }
        catch (FormatException ex)
        {
            throw new CommandException($"Invalid --start value '{StartTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z.");
        }
        try
        {
            end = string.IsNullOrEmpty(EndTime)
                ? DateTime.UtcNow
                : DateTime.Parse(EndTime, CultureInfo.InvariantCulture,
                    DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
        }
        catch (FormatException ex)
        {
            throw new CommandException($"Invalid --end value '{EndTime}': {ex.Message}. Expected ISO 8601 UTC format, e.g. 2026-01-15T08:00:00Z.");
        }
        AggregateType aggregateType = default;
        if (!string.IsNullOrEmpty(Aggregate))
        {
            try
            {
                aggregateType = ParseAggregateType(Aggregate);
            }
            catch (ArgumentException ex)
            {
                throw new CommandException($"Invalid --aggregate value: {ex.Message}");
            }
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var nodeId = NodeIdParser.ParseRequired(NodeId);
            var start = string.IsNullOrEmpty(StartTime)
                ? DateTime.UtcNow.AddHours(-24)
                : DateTime.Parse(StartTime, CultureInfo.InvariantCulture,
                    DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
            var end = string.IsNullOrEmpty(EndTime)
                ? DateTime.UtcNow
                : DateTime.Parse(EndTime, CultureInfo.InvariantCulture,
                    DateTimeStyles.AssumeUniversal | DateTimeStyles.AdjustToUniversal);
            IReadOnlyList<DataValue> values;
            if (string.IsNullOrEmpty(Aggregate))
@@ -88,7 +132,6 @@ public class HistoryReadCommand : CommandBase
            }
            else
            {
                var aggregateType = ParseAggregateType(Aggregate);
                await console.Output.WriteLineAsync(
                    $"History for {NodeId} ({Aggregate}, interval={IntervalMs}ms)");
                values = await service.HistoryReadAggregateAsync(
@@ -1,5 +1,7 @@
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
 using ZB.MOM.WW.OtOpcUa.Client.Shared;
@@ -29,13 +31,23 @@ public class ReadCommand : CommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        NodeId nodeId;
        try
        {
            nodeId = NodeIdParser.ParseRequired(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var nodeId = NodeIdParser.ParseRequired(NodeId);
            var value = await service.ReadValueAsync(nodeId, ct);
            await console.Output.WriteLineAsync($"Node:        {NodeId}");
@@ -1,6 +1,7 @@
 using System.Collections.Concurrent;
 using System.Threading.Channels;
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
@@ -12,42 +13,92 @@ namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 [Command("subscribe", Description = "Monitor a node for value changes")]
 public class SubscribeCommand : CommandBase
 {
    /// <summary>
    /// Creates the subscribe command used to monitor a node (or a subtree of nodes) for data-change
    /// notifications.
    /// </summary>
    /// <param name="factory">The factory that creates the shared client service for the command run.</param>
    public SubscribeCommand(IOpcUaClientServiceFactory factory) : base(factory)
    {
    }
    /// <summary>
    /// Gets the node ID to monitor. When <see cref="Recursive" /> is set, this node is the browse root
    /// and every <c>Variable</c> child it reaches is subscribed.
    /// </summary>
    [CommandOption("node", 'n', Description = "Node ID to monitor", IsRequired = true)]
    public string NodeId { get; init; } = default!;
    /// <summary>
    /// Gets the sampling interval, in milliseconds, requested for every monitored item.
    /// </summary>
    [CommandOption("interval", 'i', Description = "Sampling interval in milliseconds")]
    public int Interval { get; init; } = 1000;
    /// <summary>
    /// Gets a value indicating whether the command should browse from <see cref="NodeId" />
    /// and subscribe to every <c>Variable</c> in the subtree.
    /// </summary>
    [CommandOption("recursive", 'r', Description = "Browse recursively from --node and subscribe to every Variable found")]
    public bool Recursive { get; init; }
    /// <summary>
    /// Gets the maximum recursion depth applied while collecting variables when <see cref="Recursive" /> is set.
    /// </summary>
    [CommandOption("max-depth", Description = "Maximum recursion depth when --recursive is set")]
    public int MaxDepth { get; init; } = 10;
    /// <summary>
    /// Gets a value indicating whether per-update lines should be suppressed in favour of the final summary only.
    /// </summary>
    [CommandOption("quiet", 'q', Description = "Suppress per-update output; only print a final summary on Ctrl+C")]
    public bool Quiet { get; init; }
    /// <summary>
    /// Gets the duration, in seconds, before the command auto-exits and prints its summary.
    /// A value of <c>0</c> means the command runs until Ctrl+C.
    /// </summary>
    [CommandOption("duration", Description = "Auto-exit after N seconds and print summary (0 = run until Ctrl+C)")]
    public int DurationSeconds { get; init; } = 0;
    /// <summary>
    /// Gets the optional path that the command should write the final summary to on exit, in addition to stdout.
    /// </summary>
    [CommandOption("summary-file", Description = "Write summary to this file path on exit (in addition to stdout)")]
    public string? SummaryFile { get; init; }
    /// <summary>
    /// Connects to the server, subscribes to <see cref="NodeId" /> (or its subtree when recursive),
    /// streams data-change notifications to the console, and prints a summary when the command exits.
    /// </summary>
    /// <param name="console">The CLI console used for output and cancellation handling.</param>
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        if (Interval <= 0)
            throw new CommandException($"--interval must be greater than 0 (was {Interval}).");
        if (Recursive && MaxDepth <= 0)
            throw new CommandException($"--max-depth must be greater than 0 (was {MaxDepth}).");
        if (DurationSeconds < 0)
            throw new CommandException($"--duration must be 0 or a positive number (was {DurationSeconds}).");
        NodeId rootNodeId;
        try
        {
            rootNodeId = NodeIdParser.ParseRequired(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var rootNodeId = NodeIdParser.ParseRequired(NodeId);
            var targets = new List<(NodeId nodeId, string displayPath)>();
            if (Recursive)
            {
@@ -1,4 +1,5 @@
 using CliFx.Attributes;
 using CliFx.Exceptions;
 using CliFx.Infrastructure;
 using Opc.Ua;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers;
@@ -37,14 +38,23 @@ public class WriteCommand : CommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        NodeId nodeId;
        try
        {
            nodeId = NodeIdParser.ParseRequired(NodeId);
        }
        catch (Exception ex) when (ex is FormatException or ArgumentException)
        {
            throw new CommandException($"Invalid --node value: {ex.Message}");
        }
        IOpcUaClientService? service = null;
        try
        {
            var ct = console.RegisterCancellationHandler();
            (service, _) = await CreateServiceAndConnectAsync(ct);
            var nodeId = NodeIdParser.ParseRequired(NodeId);
            // Read current value to determine type for conversion
            var currentValue = await service.ReadValueAsync(nodeId, ct);
            var typedValue = ValueConverter.ConvertValue(Value, currentValue.Value);
@@ -14,7 +14,13 @@ internal sealed class DefaultApplicationConfigurationFactory : IApplicationConfi
    public async Task<ApplicationConfiguration> CreateAsync(ConnectionSettings settings, CancellationToken ct)
    {
-        var storePath = settings.CertificateStorePath;
+        // Resolve the canonical PKI path lazily on first use so constructing a
        // ConnectionSettings instance — including the throwaway copies the client
        // service builds per failover attempt — does not touch the filesystem.
        // Callers that supply an explicit path override the default.
        var storePath = string.IsNullOrWhiteSpace(settings.CertificateStorePath)
            ? ClientStoragePaths.GetPkiPath()
            : settings.CertificateStorePath;
        var config = new ApplicationConfiguration
        {
@@ -24,9 +24,47 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
        using var client = DiscoveryClient.Create(new Uri(endpointUrl));
        var allEndpoints = client.GetEndpoints(null);
        return EndpointSelector.SelectBest(allEndpoints, endpointUrl, requestedMode);
    }
 }
 /// <summary>
 ///     Pure best-endpoint selection logic, extracted from <see cref="DefaultEndpointDiscovery"/>
 ///     so it can be unit tested without standing up a real <see cref="DiscoveryClient"/>.
 /// </summary>
 internal static class EndpointSelector
 {
    private static readonly ILogger Logger = Log.ForContext(typeof(EndpointSelector));
    /// <summary>
    ///     Picks the best endpoint from the discovery response that matches the requested
    ///     security mode, preferring <c>Basic256Sha256</c>, and rewrites the endpoint URL
    ///     host to match the user-supplied URL when the discovery response advertises a
    ///     different hostname.
    /// </summary>
    /// <param name="allEndpoints">Endpoints returned by the discovery query, in any order.</param>
    /// <param name="endpointUrl">The endpoint URL the operator supplied; supplies the hostname rewrite target.</param>
    /// <param name="requestedMode">The requested OPC UA message security mode.</param>
    /// <exception cref="InvalidOperationException">
    ///     Thrown when no endpoint matches <paramref name="requestedMode"/>; the message lists the
    ///     security mode + policy combinations the server returned so operators can diagnose mismatches.
    /// </exception>
    public static EndpointDescription SelectBest(
        IEnumerable<EndpointDescription> allEndpoints,
        string endpointUrl,
        MessageSecurityMode requestedMode)
    {
        ArgumentNullException.ThrowIfNull(allEndpoints);
        if (string.IsNullOrWhiteSpace(endpointUrl))
            throw new ArgumentException("Endpoint URL must not be null or empty.", nameof(endpointUrl));
        // Materialise once so we can both iterate and produce a diagnostic message
        // without re-running the underlying discovery enumeration.
        var endpoints = allEndpoints.ToList();
        EndpointDescription? best = null;
-        foreach (var ep in allEndpoints)
+        foreach (var ep in endpoints)
        {
            if (ep.SecurityMode != requestedMode)
                continue;
@@ -37,18 +75,21 @@ internal sealed class DefaultEndpointDiscovery : IEndpointDiscovery
                continue;
            }
            // Prefer Basic256Sha256 when multiple endpoints match the requested mode.
            if (ep.SecurityPolicyUri == SecurityPolicies.Basic256Sha256)
                best = ep;
        }
        if (best == null)
        {
-            var available = string.Join(", ", allEndpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
+            var available = string.Join(", ", endpoints.Select(e => $"{e.SecurityMode}/{e.SecurityPolicyUri}"));
            throw new InvalidOperationException(
                $"No endpoint found with security mode '{requestedMode}'. Available endpoints: {available}");
        }
-        // Rewrite endpoint URL hostname to match user-supplied hostname
+        // Rewrite endpoint URL hostname to match user-supplied hostname. Necessary
        // when the OPC UA server returns a discovery URL using a different hostname
        // (e.g. internal DNS name) than the one the operator routed to.
        var serverUri = new Uri(best.EndpointUrl);
        var requestedUri = new Uri(endpointUrl);
        if (serverUri.Host != requestedUri.Host)
@@ -73,6 +73,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
        var writeCollection = new WriteValueCollection { writeValue };
        var response = await _session.WriteAsync(null, writeCollection, ct);
        // A malformed or service-level-faulted response can come back with an empty
        // Results collection alongside a service fault. Surface the service result
        // (or BadUnexpectedError) rather than letting Results[0] throw
        // IndexOutOfRangeException upstream.
        if (response.Results == null || response.Results.Count == 0)
        {
            var serviceResult = response.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
            throw new ServiceResultException(serviceResult,
                $"Write response contained no results for node {nodeId}.");
        }
        return response.Results[0];
    }
@@ -143,15 +154,18 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            if (continuationPoint != null)
                nodesToRead[0].ContinuationPoint = continuationPoint;
-            _session.HistoryRead(
+            // Use the async overload so this method is genuinely asynchronous,
            // honors the cancellation token, and does not block the caller's thread
            // (which would block the UI dispatcher for client.ui consumers).
            var response = await _session.HistoryReadAsync(
                null,
                new ExtensionObject(details),
                TimestampsToReturn.Source,
                continuationPoint != null,
                nodesToRead,
-                out var results,
+                ct).ConfigureAwait(false);
                out _);
            var results = response.Results;
            if (results == null || results.Count == 0)
                break;
@@ -186,15 +200,17 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            new HistoryReadValueId { NodeId = nodeId }
        };
-        _session.HistoryRead(
+        // Use the async overload so the method honors the cancellation token and
        // does not block on a synchronous service round-trip.
        var response = await _session.HistoryReadAsync(
            null,
            new ExtensionObject(details),
            TimestampsToReturn.Source,
            false,
            nodesToRead,
-            out var results,
+            ct).ConfigureAwait(false);
            out _);
        var results = response.Results;
        var allValues = new List<DataValue>();
        if (results != null && results.Count > 0)
@@ -229,7 +245,9 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
    {
        try
        {
-            if (_session.Connected) _session.Close();
+            // Use the async overload so the caller does not block on the close
            // service round-trip and the cancellation token is honored.
            if (_session.Connected) await _session.CloseAsync(ct).ConfigureAwait(false);
        }
        catch (Exception ex)
        {
@@ -270,6 +288,15 @@ internal sealed class DefaultSessionAdapter : ISessionAdapter
            },
            ct);
        // An empty Results collection paired with a service fault must surface as
        // a ServiceResultException, not an IndexOutOfRangeException from Results[0].
        if (result.Results == null || result.Results.Count == 0)
        {
            var serviceResult = result.ResponseHeader?.ServiceResult.Code ?? StatusCodes.BadUnexpectedError;
            throw new ServiceResultException(serviceResult,
                $"Call response contained no results for method {methodId} on {objectId}.");
        }
        var callResult = result.Results[0];
        if (StatusCode.IsBad(callResult.StatusCode))
            throw new ServiceResultException(callResult.StatusCode);
@@ -96,6 +96,11 @@ public interface IOpcUaClientService : IDisposable
    /// <param name="eventId">The event identifier returned by the OPC UA server for the alarm event.</param>
    /// <param name="comment">The operator acknowledgment comment to write with the method call.</param>
    /// <param name="ct">The cancellation token that aborts the acknowledgment request.</param>
    /// <returns>
    /// <see cref="StatusCodes.Good"/> on success, or the server's bad <see cref="StatusCode"/>
    /// (from the underlying <see cref="ServiceResultException"/>) when the acknowledge call
    /// returns a bad result. Other transport-level failures still surface as exceptions.
    /// </returns>
    Task<StatusCode> AcknowledgeAlarmAsync(string conditionNodeId, byte[] eventId, string comment, CancellationToken ct = default);
    /// <summary>
@@ -41,11 +41,13 @@ public sealed class ConnectionSettings
    public bool AutoAcceptCertificates { get; set; } = true;
    /// <summary>
-    ///     Path to the certificate store. Defaults to a subdirectory under LocalApplicationData
+    ///     Path to the certificate store. Defaults to <see cref="string.Empty"/>; the
-    ///     resolved via <see cref="ClientStoragePaths"/> so the one-shot legacy-folder migration
+    ///     consuming application configuration factory resolves the canonical path via
-    ///     runs before the path is returned.
+    ///     <see cref="ClientStoragePaths.GetPkiPath"/> lazily on first connect, so
    ///     constructing settings — including the throwaway copies built per failover
    ///     attempt — does not touch disk or run the legacy-folder migration probe.
    /// </summary>
-    public string CertificateStorePath { get; set; } = ClientStoragePaths.GetPkiPath();
+    public string CertificateStorePath { get; set; } = string.Empty;
    /// <summary>
    ///     Validates the settings and throws if any required values are missing or invalid.
@@ -353,11 +353,24 @@ public sealed class OpcUaClientService : IOpcUaClientService
            : NodeId.Parse(conditionNodeId + ".Condition");
        var acknowledgeMethodId = MethodIds.AcknowledgeableConditionType_Acknowledge;
-        await _session!.CallMethodAsync(
+        // CallMethodAsync throws ServiceResultException on a bad call result;
-            conditionObjId,
+        // surface that as the returned StatusCode so callers using the documented
-            acknowledgeMethodId,
+        // `Task<StatusCode>` contract (e.g. `if (StatusCode.IsBad(result))`) see
-            [eventId, new LocalizedText(comment)],
+        // the failure instead of an uncaught exception they did not anticipate.
-            ct);
+        try
        {
            await _session!.CallMethodAsync(
                conditionObjId,
                acknowledgeMethodId,
                [eventId, new LocalizedText(comment)],
                ct);
        }
        catch (ServiceResultException ex)
        {
            Logger.Warning(ex, "Failed to acknowledge alarm on {ConditionId} (status {Status})",
                conditionNodeId, ex.StatusCode);
            return ex.StatusCode;
        }
        Logger.Debug("Acknowledged alarm on {ConditionId}", conditionNodeId);
        return StatusCodes.Good;
@@ -68,7 +68,10 @@ public static class SnapshotFormatter
        int tagW = rows.Length == 0 ? "TAG".Length : Math.Max("TAG".Length, rows.Max(r => r.Tag.Length));
        int valW = rows.Length == 0 ? "VALUE".Length : Math.Max("VALUE".Length, rows.Max(r => r.Value.Length));
        int statW = rows.Length == 0 ? "STATUS".Length : Math.Max("STATUS".Length, rows.Max(r => r.Status.Length));
-        // source-time column is fixed-width (ISO-8601 to ms) so no max-measurement needed.
+        // source-time is the right-most column, so it is intentionally not measured or padded;
        // when a snapshot has a non-null SourceTimestampUtc the cell is 24 chars (ISO-8601 to ms),
        // and when the timestamp is null FormatTimestamp emits "-" — the resulting unalignment is
        // harmless because nothing is appended after this column.
        var sb = new System.Text.StringBuilder();
        sb.Append("TAG".PadRight(tagW)).Append("  ")
@@ -24,6 +24,8 @@ public sealed class ProbeCommand : FocasCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work.
        ValidateOptions();
        var ct = console.RegisterCancellationHandler();
        var probeTag = new FocasTagDefinition(
@@ -34,24 +36,20 @@ public sealed class ProbeCommand : FocasCommandBase
            Writable: false);
        var options = BuildOptions([probeTag]);
        // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.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 FocasDriver(options, DriverInstanceId);
-        try
+        await driver.InitializeAsync("{}", ct);
-        {
+        var snapshot = await driver.ReadAsync(["__probe"], ct);
-            await driver.InitializeAsync("{}", ct);
+        var health = driver.GetHealth();
            var snapshot = await driver.ReadAsync(["__probe"], ct);
            var health = driver.GetHealth();
-            await console.Output.WriteLineAsync($"CNC:          {CncHost}:{CncPort}");
+        await console.Output.WriteLineAsync($"CNC:          {CncHost}:{CncPort}");
-            await console.Output.WriteLineAsync($"Series:       {Series}");
+        await console.Output.WriteLineAsync($"Series:       {Series}");
-            await console.Output.WriteLineAsync($"Health:       {health.State}");
+        await console.Output.WriteLineAsync($"Health:       {health.State}");
-            if (health.LastError is { } err)
+        if (health.LastError is { } err)
-                await console.Output.WriteLineAsync($"Last error:   {err}");
+            await console.Output.WriteLineAsync($"Last error:   {err}");
-            await console.Output.WriteLineAsync();
+        await console.Output.WriteLineAsync();
-            await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
+        await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
        }
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
 }
@@ -23,6 +23,8 @@ public sealed class ReadCommand : FocasCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work.
        ValidateOptions();
        var ct = console.RegisterCancellationHandler();
        var tagName = SynthesiseTagName(Address, DataType);
@@ -34,17 +36,13 @@ public sealed class ReadCommand : FocasCommandBase
            Writable: false);
        var options = BuildOptions([tag]);
        // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.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 FocasDriver(options, DriverInstanceId);
-        try
+        await driver.InitializeAsync("{}", ct);
-        {
+        var snapshot = await driver.ReadAsync([tagName], ct);
-            await driver.InitializeAsync("{}", ct);
+        await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
            var snapshot = await driver.ReadAsync([tagName], ct);
            await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
        }
        finally
        {
            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
    internal static string SynthesiseTagName(string address, FocasDataType type)
@@ -25,6 +25,10 @@ public sealed class SubscribeCommand : FocasCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        // Driver.FOCAS.Cli-003: validate numeric option ranges (including the subscribe-only
        // --interval-ms) before any driver work so a zero/negative interval surfaces as a
        // clean CommandException rather than a tight-spinning poll loop.
        ValidateOptions(IntervalMs);
        var ct = console.RegisterCancellationHandler();
        var tagName = ReadCommand.SynthesiseTagName(Address, DataType);
@@ -36,24 +40,59 @@ public sealed class SubscribeCommand : FocasCommandBase
            Writable: false);
        var options = BuildOptions([tag]);
        // Driver.FOCAS.Cli-004: `await using` is the sole driver-disposal mechanism — FocasDriver.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 FocasDriver(options, DriverInstanceId);
        // Driver.FOCAS.Cli-002: serialize console writes from the PollGroupEngine background
        // thread so overlapping poll ticks (and the "Subscribed to ..." banner from the CliFx
        // invocation thread) can't interleave partial lines.
        var writeLock = new object();
        ISubscriptionHandle? handle = null;
        try
        {
            await driver.InitializeAsync("{}", ct);
            // Driver.FOCAS.Cli-002: 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 not documented as thread-safe
            // so we serialize against the banner write via writeLock.
            driver.OnDataChange += (_, e) =>
            {
-                var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
+                // Swallow + log write failures so a transient stdout error (closed pipe, IO
-                    $"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
+                // exception on a redirected stream) cannot tear down the poll-engine
-                    $"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
+                // background loop. Without this guard the unhandled exception would fault
-                console.Output.WriteLine(line);
+                // 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);
-            await console.Output.WriteLineAsync(
+            // Driver.FOCAS.Cli-002: hold the lock around the banner write so the first
-                $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
+            // poll-driven change line from the driver tick thread can't interleave with
            // this banner.
            lock (writeLock)
            {
                console.Output.WriteLine(
                    $"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
            }
            try
            {
                await Task.Delay(System.Threading.Timeout.InfiniteTimeSpan, ct);
@@ -67,10 +106,16 @@ public sealed class SubscribeCommand : FocasCommandBase
        {
            if (handle is not null)
            {
                // Driver.FOCAS.Cli-002: detach the OnDataChange handler before unsubscribe +
                // disposal for symmetry with the handle teardown, so a future refactor that
                // reuses the driver after the subscribe verb returns wouldn't leak a
                // dangling subscription.
                // (Single anonymous handler instance is captured implicitly by `await using`
                // disposing the driver immediately afterwards; the unsubscribe + dispose
                // sequence is what really cleans up here.)
                try { await driver.UnsubscribeAsync(handle, CancellationToken.None); }
                catch { /* teardown best-effort */ }
            }
            await driver.ShutdownAsync(CancellationToken.None);
        }
    }
 }
@@ -29,6 +29,10 @@ public sealed class WriteCommand : FocasCommandBase
    public override async ValueTask ExecuteAsync(IConsole console)
    {
        ConfigureLogging();
        // Driver.FOCAS.Cli-003: validate numeric option ranges before any driver work so
        // a zero/negative port/timeout surfaces as a clean CommandException rather than an
        // opaque downstream exception.
        ValidateOptions();
        var ct = console.RegisterCancellationHandler();
        var tagName = ReadCommand.SynthesiseTagName(Address, DataType);
@@ -42,30 +46,49 @@ public sealed class WriteCommand : FocasCommandBase
        var parsed = ParseValue(Value, DataType);
        // Driver.FOCAS.Cli-004: `await using` is the sole disposal mechanism — FocasDriver.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 FocasDriver(options, DriverInstanceId);
-        try
+        await driver.InitializeAsync("{}", ct);
-        {
+        var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
-            await driver.InitializeAsync("{}", ct);
+        await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
            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);
        }
    }
-    internal static object ParseValue(string raw, FocasDataType type) => type switch
+    /// <summary>Parse <c>--value</c> per <see cref="FocasDataType"/>, invariant culture throughout.</summary>
    /// <remarks>
    ///     Driver.FOCAS.Cli-001: numeric parses are wrapped so that malformed input
    ///     (<see cref="FormatException"/> / <see cref="OverflowException"/>) surfaces
    ///     as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than a raw
    ///     .NET stack trace — matching the friendly message the Bit path already produces.
    /// </remarks>
    internal static object ParseValue(string raw, FocasDataType type)
    {
-        FocasDataType.Bit => ParseBool(raw),
+        if (type == FocasDataType.Bit) return ParseBool(raw);
-        FocasDataType.Byte => sbyte.Parse(raw, CultureInfo.InvariantCulture),
+        if (type == FocasDataType.String) return raw;
-        FocasDataType.Int16 => short.Parse(raw, CultureInfo.InvariantCulture),
+        try
-        FocasDataType.Int32 => int.Parse(raw, CultureInfo.InvariantCulture),
+        {
-        FocasDataType.Float32 => float.Parse(raw, CultureInfo.InvariantCulture),
+            return type switch
-        FocasDataType.Float64 => double.Parse(raw, CultureInfo.InvariantCulture),
+            {
-        FocasDataType.String => raw,
+                FocasDataType.Byte    => (object)sbyte.Parse(raw, CultureInfo.InvariantCulture),
-        _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
+                FocasDataType.Int16   => (object)short.Parse(raw, CultureInfo.InvariantCulture),
-    };
+                FocasDataType.Int32   => (object)int.Parse(raw, CultureInfo.InvariantCulture),
                FocasDataType.Float32 => (object)float.Parse(raw, CultureInfo.InvariantCulture),
                FocasDataType.Float64 => (object)double.Parse(raw, CultureInfo.InvariantCulture),
                _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
            };
        }
        catch (FormatException ex)
        {
            throw new CliFx.Exceptions.CommandException(
                $"Value '{raw}' is not a valid {type}: {ex.Message}");
        }
        catch (OverflowException ex)
        {
            throw new CliFx.Exceptions.CommandException(
                $"Value '{raw}' is out of range for {type}: {ex.Message}");
        }
    }
    private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch
    {
@@ -54,4 +54,26 @@ public abstract class FocasCommandBase : DriverCommandBase
    };
    protected string DriverInstanceId => $"focas-cli-{CncHost}:{CncPort}";
    /// <summary>
    ///     Driver.FOCAS.Cli-003: validate numeric option ranges at the CLI boundary so a
    ///     zero/negative <c>--cnc-port</c>, <c>--timeout-ms</c>, or <c>--interval-ms</c>
    ///     surfaces as a clean <see cref="CliFx.Exceptions.CommandException"/> rather than
    ///     either an opaque downstream exception (invalid <c>focas://host:&lt;n&gt;</c> /
    ///     zero <c>TimeSpan</c>) or a tight-spinning poll loop. The <c>--interval-ms</c>
    ///     option is subscribe-only — pass <c>null</c> for probe/read/write so this
    ///     helper can be a single shared validator.
    /// </summary>
    protected void ValidateOptions(int? intervalMs = null)
    {
        if (CncPort < 1 || CncPort > 65535)
            throw new CliFx.Exceptions.CommandException(
                $"--cnc-port must be in the range 1..65535 (got {CncPort}).");
        if (TimeoutMs <= 0)
            throw new CliFx.Exceptions.CommandException(
                $"--timeout-ms must be positive (got {TimeoutMs}).");
        if (intervalMs is { } iv && iv <= 0)
            throw new CliFx.Exceptions.CommandException(
                $"--interval-ms must be positive (got {iv}).");
    }
 }
@@ -22,6 +22,10 @@
        <ProjectReference Include="..\..\ZB.MOM.WW.OtOpcUa.Driver.FOCAS\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.csproj"/>
    </ItemGroup>
    <ItemGroup>
        <InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests"/>
    </ItemGroup>
    <!-- CLI runs the managed WireFocasClient and talks to the CNC over TCP:8193
         directly — no Fwlib64.dll copy step needed. -->
@@ -72,8 +72,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
            MaxValues = (int)Math.Min(maxValuesPerNode, int.MaxValue),
            CorrelationId = Guid.NewGuid().ToString("N"),
        };
-        var reply = await Invoke<ReadRawRequest, ReadRawReply>(MessageKind.ReadRawRequest, MessageKind.ReadRawReply, req, cancellationToken).ConfigureAwait(false);
+        var reply = await InvokeAndClassifyAsync<ReadRawRequest, ReadRawReply>(
-        ThrowIfFailed(reply.Success, reply.Error, "ReadRaw");
+            MessageKind.ReadRawRequest, MessageKind.ReadRawReply, req,
            r => (r.Success, r.Error), "ReadRaw", cancellationToken).ConfigureAwait(false);
        return new HistoryReadResult(ToSnapshots(reply.Samples), ContinuationPoint: null);
    }
@@ -90,8 +91,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
            AggregateColumn = MapAggregate(aggregate),
            CorrelationId = Guid.NewGuid().ToString("N"),
        };
-        var reply = await Invoke<ReadProcessedRequest, ReadProcessedReply>(MessageKind.ReadProcessedRequest, MessageKind.ReadProcessedReply, req, cancellationToken).ConfigureAwait(false);
+        var reply = await InvokeAndClassifyAsync<ReadProcessedRequest, ReadProcessedReply>(
-        ThrowIfFailed(reply.Success, reply.Error, "ReadProcessed");
+            MessageKind.ReadProcessedRequest, MessageKind.ReadProcessedReply, req,
            r => (r.Success, r.Error), "ReadProcessed", cancellationToken).ConfigureAwait(false);
        return new HistoryReadResult(ToAggregateSnapshots(reply.Buckets), ContinuationPoint: null);
    }
@@ -107,8 +109,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
            TimestampsUtcTicks = ticks,
            CorrelationId = Guid.NewGuid().ToString("N"),
        };
-        var reply = await Invoke<ReadAtTimeRequest, ReadAtTimeReply>(MessageKind.ReadAtTimeRequest, MessageKind.ReadAtTimeReply, req, cancellationToken).ConfigureAwait(false);
+        var reply = await InvokeAndClassifyAsync<ReadAtTimeRequest, ReadAtTimeReply>(
-        ThrowIfFailed(reply.Success, reply.Error, "ReadAtTime");
+            MessageKind.ReadAtTimeRequest, MessageKind.ReadAtTimeReply, req,
            r => (r.Success, r.Error), "ReadAtTime", cancellationToken).ConfigureAwait(false);
        return new HistoryReadResult(AlignAtTimeSnapshots(timestampsUtc, reply.Samples), ContinuationPoint: null);
    }
@@ -167,11 +170,34 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
            MaxEvents = maxEvents,
            CorrelationId = Guid.NewGuid().ToString("N"),
        };
-        var reply = await Invoke<ReadEventsRequest, ReadEventsReply>(MessageKind.ReadEventsRequest, MessageKind.ReadEventsReply, req, cancellationToken).ConfigureAwait(false);
+        var reply = await InvokeAndClassifyAsync<ReadEventsRequest, ReadEventsReply>(
-        ThrowIfFailed(reply.Success, reply.Error, "ReadEvents");
+            MessageKind.ReadEventsRequest, MessageKind.ReadEventsReply, req,
            r => (r.Success, r.Error), "ReadEvents", cancellationToken).ConfigureAwait(false);
        return new HistoricalEventsResult(ToHistoricalEvents(reply.Events), ContinuationPoint: null);
    }
    /// <summary>
    ///     Returns a snapshot of operation counters and the single pipe channel's connection
    ///     state.
    /// </summary>
    /// <remarks>
    ///     This client owns one duplex named-pipe channel to the sidecar — it has no notion of
    ///     separate process / event connections and no per-node telemetry. The single channel's
    ///     connected state is reported for both <see cref="HistorianHealthSnapshot.ProcessConnectionOpen"/>
    ///     and <see cref="HistorianHealthSnapshot.EventConnectionOpen"/>, and
    ///     <see cref="HistorianHealthSnapshot.ActiveProcessNode"/> /
    ///     <see cref="HistorianHealthSnapshot.ActiveEventNode"/> /
    ///     <see cref="HistorianHealthSnapshot.Nodes"/> are intentionally null/empty. Consumers
    ///     that need to distinguish two connections should read another driver. (Finding 010.)
    ///     <para>
    ///         All six counter fields (TotalQueries, TotalSuccesses, TotalFailures,
    ///         ConsecutiveFailures, LastSuccessTime, LastFailureTime, LastError) are mutated
    ///         exclusively under <c>_healthLock</c>, so the snapshot is internally consistent —
    ///         in particular <c>TotalSuccesses + TotalFailures == TotalQueries</c> at every
    ///         observed snapshot (a call that has started but not yet completed has not
    ///         incremented any counter). (Finding 003 / 004.)
    ///     </para>
    /// </remarks>
    public HistorianHealthSnapshot GetHealthSnapshot()
    {
        lock (_healthLock)
@@ -233,8 +259,9 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
        try
        {
-            var reply = await Invoke<WriteAlarmEventsRequest, WriteAlarmEventsReply>(
+            var reply = await InvokeAsync<WriteAlarmEventsRequest, WriteAlarmEventsReply>(
-                MessageKind.WriteAlarmEventsRequest, MessageKind.WriteAlarmEventsReply, req, cancellationToken).ConfigureAwait(false);
+                MessageKind.WriteAlarmEventsRequest, MessageKind.WriteAlarmEventsReply, req,
                r => (r.Success, r.Error), cancellationToken).ConfigureAwait(false);
            // Whole-call failure → transient retry for every event in the batch.
            if (!reply.Success)
@@ -279,69 +306,79 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
    // ===== Helpers =====
-    private async Task<TReply> Invoke<TRequest, TReply>(
+    /// <summary>
-        MessageKind requestKind, MessageKind expectedReplyKind, TRequest request, CancellationToken ct)
+    ///     Sends one request through the channel and records the outcome (transport success or
    ///     transport failure) under a single <c>_healthLock</c> acquisition that also bumps
    ///     <c>_totalQueries</c>. Sidecar-level success / failure is NOT classified here — the
    ///     caller passes that through <see cref="InvokeAndClassifyAsync"/> instead. (Finding
    ///     003 / 004: all six counter fields share one synchronization mechanism so a snapshot
    ///     can never observe a torn state.)
    /// </summary>
    private async Task<TReply> InvokeAsync<TRequest, TReply>(
        MessageKind requestKind, MessageKind expectedReplyKind, TRequest request,
        Func<TReply, (bool ok, string? error)> evaluate, CancellationToken ct)
        where TReply : class
    {
        Interlocked.Increment(ref _totalQueries);
        try
        {
            var reply = await _channel.InvokeAsync<TRequest, TReply>(requestKind, expectedReplyKind, request, ct).ConfigureAwait(false);
-            RecordSuccess();
+            // Classify transport+sidecar in one lock so TotalQueries/TotalSuccesses/
            // TotalFailures move together and no intermediate "success-then-undo" state is
            // visible to a concurrent GetHealthSnapshot.
            var (ok, error) = evaluate(reply);
            RecordOutcome(ok, error);
            return reply;
        }
        catch (Exception ex)
        {
-            RecordFailure(ex.Message);
+            RecordOutcome(success: false, ex.Message);
            throw;
        }
    }
-    private void RecordSuccess()
+    /// <summary>
    ///     Convenience wrapper around <see cref="InvokeAsync"/> that throws
    ///     <see cref="InvalidOperationException"/> on a sidecar-reported failure. Used by the
    ///     <see cref="IHistorianDataSource"/> read methods.
    /// </summary>
    private async Task<TReply> InvokeAndClassifyAsync<TRequest, TReply>(
        MessageKind requestKind, MessageKind expectedReplyKind, TRequest request,
        Func<TReply, (bool ok, string? error)> evaluate, string op, CancellationToken ct)
        where TReply : class
    {
-        lock (_healthLock)
+        var reply = await InvokeAsync<TRequest, TReply>(requestKind, expectedReplyKind, request, evaluate, ct).ConfigureAwait(false);
        var (ok, error) = evaluate(reply);
        if (!ok)
        {
            _totalSuccesses++;
            _consecutiveFailures = 0;
            _lastSuccessUtc = DateTime.UtcNow;
        }
    }
    private void RecordFailure(string message)
    {
        lock (_healthLock)
        {
            _totalFailures++;
            _consecutiveFailures++;
            _lastFailureUtc = DateTime.UtcNow;
            _lastError = message;
        }
    }
    private void ThrowIfFailed(bool success, string? error, string op)
    {
        if (!success)
        {
            // Sidecar-reported failure counts as an operation failure even though the
            // transport delivered a reply. The Invoke wrapper already recorded transport
            // success — undo that and record the failure so the health snapshot reflects
            // operation-level success rates rather than just connectivity.
            ReclassifySuccessAsFailure(error);
            throw new InvalidOperationException(
                $"Sidecar {op} failed: {error ?? "<no message>"}.");
        }
        return reply;
    }
-    private void ReclassifySuccessAsFailure(string? message)
+    /// <summary>
    ///     Records the outcome of a single call — increments <c>_totalQueries</c> and exactly
    ///     one of <c>_totalSuccesses</c> / <c>_totalFailures</c> under a single
    ///     <c>_healthLock</c> acquisition. (Findings 003 + 004.)
    /// </summary>
    private void RecordOutcome(bool success, string? error)
    {
        lock (_healthLock)
        {
-            // Transport-level RecordSuccess happened a moment ago; reverse it.
+            _totalQueries++;
-            _totalSuccesses--;
+            if (success)
-            _totalFailures++;
+            {
-            _consecutiveFailures++;
+                _totalSuccesses++;
-            _lastFailureUtc = DateTime.UtcNow;
+                _consecutiveFailures = 0;
-            _lastError = message;
+                _lastSuccessUtc = DateTime.UtcNow;
            }
            else
            {
                _totalFailures++;
                _consecutiveFailures++;
                _lastFailureUtc = DateTime.UtcNow;
                _lastError = error;
            }
        }
    }
@@ -452,9 +489,12 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
    /// <summary>
    ///     Synchronous dispose required by <see cref="IDisposable"/> on
-    ///     <see cref="IHistorianDataSource"/>. The underlying channel's async cleanup is
+    ///     <see cref="IHistorianDataSource"/>. The underlying channel's async cleanup runs
-    ///     non-blocking (just resets transport state + disposes streams), so the
+    ///     <see cref="System.IO.Pipes.NamedPipeClientStream"/> teardown, which can block briefly
-    ///     GetAwaiter()/GetResult() bridge is safe.
+    ///     on OS handle release — strictly speaking it is not non-blocking — but the
    ///     <c>GetAwaiter()/GetResult()</c> bridge is deadlock-safe because the cleanup never
    ///     awaits a captured <see cref="System.Threading.SynchronizationContext"/> nor takes any
    ///     lock that the caller could hold. (Finding 010.)
    /// </summary>
    public void Dispose() => _channel.DisposeAsync().AsTask().GetAwaiter().GetResult();
 }
@@ -3,24 +3,28 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client;
 /// <summary>
 ///     Connection options for <see cref="WonderwareHistorianClient"/>.
 /// </summary>
 /// <remarks>
 ///     <para>
 ///         <b>Retry / backoff ownership (finding 006):</b> this module performs exactly one
 ///         in-place transport reconnect inside <c>PipeChannel.InvokeAsync</c> with no delay,
 ///         and does NOT implement exponential reconnect backoff. Broader retry/backoff is the
 ///         caller's responsibility — the alarm drain worker
 ///         (<c>Core.AlarmHistorian.SqliteStoreAndForwardSink</c>) and the read-side
 ///         history router are expected to layer their own backoff on top.
 ///     </para>
 /// </remarks>
 /// <param name="PipeName">Named-pipe name the sidecar listens on (matches the sidecar's <c>OTOPCUA_HISTORIAN_PIPE</c>).</param>
 /// <param name="SharedSecret">Per-process shared secret the sidecar will verify in the Hello frame.</param>
 /// <param name="PeerName">Diagnostic peer identifier sent in Hello — typically the OtOpcUa instance id.</param>
 /// <param name="ConnectTimeout">Cap on the named-pipe connect + Hello round trip on each (re)connect.</param>
 /// <param name="CallTimeout">Cap on a single read/write call once connected.</param>
 /// <param name="ReconnectInitialBackoff">Backoff between the first failed reconnect attempts.</param>
 /// <param name="ReconnectMaxBackoff">Upper bound on the exponential backoff between reconnects.</param>
 public sealed record WonderwareHistorianClientOptions(
    string PipeName,
    string SharedSecret,
    string PeerName = "OtOpcUa",
    TimeSpan? ConnectTimeout = null,
-    TimeSpan? CallTimeout = null,
+    TimeSpan? CallTimeout = null)
    TimeSpan? ReconnectInitialBackoff = null,
    TimeSpan? ReconnectMaxBackoff = null)
 {
    public TimeSpan EffectiveConnectTimeout => ConnectTimeout ?? TimeSpan.FromSeconds(10);
    public TimeSpan EffectiveCallTimeout => CallTimeout ?? TimeSpan.FromSeconds(30);
    public TimeSpan EffectiveReconnectInitialBackoff => ReconnectInitialBackoff ?? TimeSpan.FromMilliseconds(500);
    public TimeSpan EffectiveReconnectMaxBackoff => ReconnectMaxBackoff ?? TimeSpan.FromSeconds(30);
 }
@@ -0,0 +1,165 @@
 using CliFx.Exceptions;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
 namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
 /// <summary>
 /// Regression tests for Client.CLI-003: numeric command options must validate their ranges
 /// and report a clean operator-facing error instead of silently passing bad values to the SDK.
 /// </summary>
 public class CommandRangeValidationTests
 {
    [Fact]
    public async Task BrowseCommand_NegativeDepth_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new BrowseCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            Depth = -1
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--depth");
    }
    [Fact]
    public async Task BrowseCommand_ZeroDepth_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new BrowseCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            Depth = 0
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--depth");
    }
    [Fact]
    public async Task SubscribeCommand_ZeroInterval_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            Interval = 0
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--interval");
    }
    [Fact]
    public async Task SubscribeCommand_NegativeInterval_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            Interval = -50
        };
        using var console = TestConsoleHelper.CreateConsole();
        await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
    }
    [Fact]
    public async Task SubscribeCommand_RecursiveZeroMaxDepth_ThrowsCommandException()
    {
        // --max-depth only matters when --recursive is set; validation is scoped to that combination.
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            Recursive = true,
            MaxDepth = 0
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--max-depth");
    }
    [Fact]
    public async Task SubscribeCommand_NegativeDuration_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            DurationSeconds = -5
        };
        using var console = TestConsoleHelper.CreateConsole();
        await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
    }
    [Fact]
    public async Task AlarmsCommand_ZeroInterval_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new AlarmsCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            Interval = 0
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--interval");
    }
    [Fact]
    public async Task HistoryReadCommand_NegativeMax_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new HistoryReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            MaxValues = -10
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--max");
    }
    [Fact]
    public async Task HistoryReadCommand_ZeroInterval_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new HistoryReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            Aggregate = "Average",
            IntervalMs = 0
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--interval");
    }
 }
@@ -0,0 +1,59 @@
 using Opc.Ua;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
 using ZB.MOM.WW.OtOpcUa.Client.Shared.Models;
 namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
 /// <summary>
 /// Regression tests for Client.CLI-009: long-running commands must detach their event handlers
 /// from the service before the command finishes, so notifications fired during teardown don't
 /// land in a disposed console.
 /// </summary>
 public class EventHandlerLifecycleTests
 {
    [Fact]
    public async Task SubscribeCommand_AfterExit_DataChangedEventHasNoSubscribers()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=Node",
            DurationSeconds = 1
        };
        using var console = TestConsoleHelper.CreateConsole();
        await command.ExecuteAsync(console);
        // The fake's event should have no subscribers after the command completes — every
        // handler attached by the command must have been detached during teardown.
        fakeService.HasDataChangedSubscribers.ShouldBeFalse(
            "SubscribeCommand must detach its DataChanged handler before returning.");
    }
    [Fact]
    public async Task AlarmsCommand_AfterExit_AlarmEventHasNoSubscribers()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new AlarmsCommand(factory)
        {
            Url = "opc.tcp://localhost:4840"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var task = Task.Run(async () => await command.ExecuteAsync(console));
        await Task.Delay(150);
        console.RequestCancellation();
        await task;
        fakeService.HasAlarmEventSubscribers.ShouldBeFalse(
            "AlarmsCommand must detach its AlarmEvent handler before returning.");
    }
 }
@@ -55,6 +55,14 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
        new("ns=2;s=Node2", "Node2", "Variable", false)
    };
    /// <summary>
    /// Optional per-parent-node browse results. When a key matches the requested parent's
    /// <see cref="NodeId.ToString" />, this dictionary takes precedence over <see cref="BrowseResults" />.
    /// Tests exercising recursive walks (Client.CLI-010) use it to model a real subtree whose
    /// child node ids do not collide on descent.
    /// </summary>
    public Dictionary<string, IReadOnlyList<BrowseResult>> BrowseResultsByParent { get; } = new();
    public IReadOnlyList<DataValue> HistoryReadResult { get; set; } = new List<DataValue>
    {
        new(new Variant(10.0), StatusCodes.Good, DateTime.UtcNow.AddHours(-1), DateTime.UtcNow),
@@ -68,6 +76,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
    public Exception? ReadException { get; set; }
    public Exception? WriteException { get; set; }
    public Exception? ConditionRefreshException { get; set; }
    public Exception? SubscribeException { get; set; }
    /// <inheritdoc />
    public bool IsConnected => ConnectCalled && !DisconnectCalled;
@@ -84,6 +93,12 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
    /// <inheritdoc />
    public event EventHandler<ConnectionStateChangedEventArgs>? ConnectionStateChanged;
    /// <summary>True when at least one handler is attached to <see cref="DataChanged" />.</summary>
    public bool HasDataChangedSubscribers => DataChanged != null;
    /// <summary>True when at least one handler is attached to <see cref="AlarmEvent" />.</summary>
    public bool HasAlarmEventSubscribers => AlarmEvent != null;
    /// <inheritdoc />
    public Task<ConnectionInfo> ConnectAsync(ConnectionSettings settings, CancellationToken ct = default)
    {
@@ -120,6 +135,9 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
    public Task<IReadOnlyList<BrowseResult>> BrowseAsync(NodeId? parentNodeId = null, CancellationToken ct = default)
    {
        BrowseNodeIds.Add(parentNodeId);
        var key = parentNodeId?.ToString();
        if (key != null && BrowseResultsByParent.TryGetValue(key, out var keyed))
            return Task.FromResult(keyed);
        return Task.FromResult(BrowseResults);
    }
@@ -127,6 +145,7 @@ public sealed class FakeOpcUaClientService : IOpcUaClientService
    public Task SubscribeAsync(NodeId nodeId, int intervalMs = 1000, CancellationToken ct = default)
    {
        SubscribeCalls.Add((nodeId, intervalMs));
        if (SubscribeException != null) throw SubscribeException;
        return Task.CompletedTask;
    }
@@ -105,7 +105,7 @@ public class HistoryReadCommandTests
    }
    [Fact]
-    public async Task Execute_InvalidAggregate_ThrowsArgumentException()
+    public async Task Execute_InvalidAggregate_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
@@ -117,7 +117,10 @@ public class HistoryReadCommandTests
        };
        using var console = TestConsoleHelper.CreateConsole();
-        await Should.ThrowAsync<ArgumentException>(async () => await command.ExecuteAsync(console));
+        // Operator-input errors now surface as CliFx CommandException (was ArgumentException)
        // per Client.CLI-006 so malformed input prints a clean CLI error instead of a stack trace.
        await Should.ThrowAsync<CliFx.Exceptions.CommandException>(
            async () => await command.ExecuteAsync(console));
    }
    [Fact]
@@ -0,0 +1,98 @@
 using CliFx.Exceptions;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
 namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
 /// <summary>
 /// Regression tests for Client.CLI-006: predictable operator-input errors must surface as
 /// CliFx CommandException with a clean message, not raw FormatException/ArgumentException
 /// with a stack trace.
 /// </summary>
 public class InputValidationErrorsTests
 {
    [Fact]
    public async Task HistoryReadCommand_InvalidStartTime_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new HistoryReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            StartTime = "not-a-date"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--start");
    }
    [Fact]
    public async Task HistoryReadCommand_InvalidEndTime_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new HistoryReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            EndTime = "garbage"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("--end");
    }
    [Fact]
    public async Task HistoryReadCommand_InvalidAggregate_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new HistoryReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=N",
            Aggregate = "NotARealAggregate"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("aggregate", Case.Insensitive);
    }
    [Fact]
    public async Task ReadCommand_InvalidNodeId_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new ReadCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "not-a-node-id"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("node", Case.Insensitive);
    }
    [Fact]
    public async Task SubscribeCommand_InvalidNodeId_ThrowsCommandException()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "not-a-node-id"
        };
        using var console = TestConsoleHelper.CreateConsole();
        var ex = await Should.ThrowAsync<CommandException>(async () => await command.ExecuteAsync(console));
        ex.Message.ShouldContain("node", Case.Insensitive);
    }
 }
@@ -0,0 +1,55 @@
 using Serilog;
 using Serilog.Core;
 using Serilog.Events;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
 namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
 /// <summary>
 /// Regression test for Client.CLI-007: ConfigureLogging must dispose the previously assigned
 /// Log.Logger before replacing it, so repeated CLI invocations do not leak sinks.
 /// </summary>
 public class LoggerLifecycleTests
 {
    [Fact]
    public async Task ConfigureLogging_DisposesPreviousLogger_BeforeReassigning()
    {
        // Install a tracker logger before the command runs.
        var trackerSink = new DisposeTrackingSink();
        var trackerLogger = new LoggerConfiguration()
            .WriteTo.Sink(trackerSink)
            .CreateLogger();
        Log.Logger = trackerLogger;
        try
        {
            var fakeService = new FakeOpcUaClientService();
            var factory = new FakeOpcUaClientServiceFactory(fakeService);
            var command = new ConnectCommand(factory)
            {
                Url = "opc.tcp://localhost:4840"
            };
            using var console = TestConsoleHelper.CreateConsole();
            await command.ExecuteAsync(console);
            // The command's ConfigureLogging should have disposed the tracker logger we installed.
            trackerSink.Disposed.ShouldBeTrue(
                "Previous Log.Logger should be disposed via Log.CloseAndFlush() before ConfigureLogging assigns a new one.");
        }
        finally
        {
            Log.CloseAndFlush();
        }
    }
    private sealed class DisposeTrackingSink : ILogEventSink, IDisposable
    {
        public bool Disposed { get; private set; }
        public void Emit(LogEvent logEvent) { }
        public void Dispose() => Disposed = true;
    }
 }
@@ -0,0 +1,258 @@
 using Opc.Ua;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands;
 using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes;
 using BrowseResult = ZB.MOM.WW.OtOpcUa.Client.Shared.Models.BrowseResult;
 namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests;
 /// <summary>
 /// Regression tests for SubscribeCommand summary bucketing, --duration, --quiet, --summary-file,
 /// and recursive collection. Covers Client.CLI-002, -009, and -010.
 /// </summary>
 public class SubscribeCommandSummaryTests
 {
    [Fact]
    public async Task Summary_NodeWithNoUpdate_IsCountedAsNeverNotAsNeverWentBad()
    {
        // Client.CLI-002: A node that received no updates at all is "never received an update",
        // NOT a "suspect that never went bad".
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=SilentNode",
            DurationSeconds = 1
        };
        using var console = TestConsoleHelper.CreateConsole();
        await command.ExecuteAsync(console);
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("No update received at all:      1");
        output.ShouldContain("NEVER went bad (suspect):       0");
        // The "suspect" detail header should not appear when the suspect list is empty.
        output.ShouldNotContain("--- Nodes that NEVER received a bad-quality update (suspect) ---");
        // The "never received update" detail header should appear.
        output.ShouldContain("--- Nodes that never received an update at all ---");
    }
    [Fact]
    public async Task Summary_NodeReceivedOnlyGoodValues_IsCountedAsNeverWentBad()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=GoodNode",
            // Use --duration so the command auto-exits.
            DurationSeconds = 2
        };
        using var console = TestConsoleHelper.CreateConsole();
        var runTask = Task.Run(async () => await command.ExecuteAsync(console));
        // Wait until the subscription is registered by the fake.
        await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
        fakeService.RaiseDataChanged(
            "ns=2;s=GoodNode",
            new DataValue(new Variant(42), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow));
        await runTask;
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("NEVER went bad (suspect):       1");
        output.ShouldContain("Last status GOOD:               1");
        output.ShouldContain("No update received at all:      0");
    }
    [Fact]
    public async Task Summary_NodeReceivedBadValue_IsCountedAsEverWentBad()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=BadNode",
            DurationSeconds = 2
        };
        using var console = TestConsoleHelper.CreateConsole();
        var runTask = Task.Run(async () => await command.ExecuteAsync(console));
        await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
        fakeService.RaiseDataChanged(
            "ns=2;s=BadNode",
            new DataValue(Variant.Null, StatusCodes.BadDeviceFailure, DateTime.UtcNow, DateTime.UtcNow));
        await runTask;
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("Ever went BAD during window:    1");
        output.ShouldContain("NEVER went bad (suspect):       0");
    }
    [Fact]
    public async Task Duration_ZeroOrPositive_AutoExits()
    {
        // --duration > 0 should make the command exit on its own without needing cancellation.
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=AutoExit",
            DurationSeconds = 1
        };
        using var console = TestConsoleHelper.CreateConsole();
        var sw = System.Diagnostics.Stopwatch.StartNew();
        await command.ExecuteAsync(console);
        sw.Stop();
        sw.Elapsed.ShouldBeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(900));
        sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(10));
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("==================== SUMMARY ====================");
    }
    [Fact]
    public async Task Quiet_SuppressesPerUpdateOutputButPrintsSummary()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=Quiet",
            Quiet = true,
            DurationSeconds = 2
        };
        using var console = TestConsoleHelper.CreateConsole();
        var runTask = Task.Run(async () => await command.ExecuteAsync(console));
        await WaitForAsync(() => fakeService.SubscribeCalls.Count == 1);
        fakeService.RaiseDataChanged(
            "ns=2;s=Quiet",
            new DataValue(new Variant(1.0), StatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow));
        await runTask;
        var output = TestConsoleHelper.GetOutput(console);
        // No per-update "value = ..." line should appear because --quiet was set.
        output.ShouldNotContain(" = 1 (");
        // Summary section is still printed.
        output.ShouldContain("==================== SUMMARY ====================");
    }
    [Fact]
    public async Task SummaryFile_WritesSummaryToDisk()
    {
        var fakeService = new FakeOpcUaClientService();
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var tempFile = Path.Combine(Path.GetTempPath(), $"otopcua-cli-summary-{Guid.NewGuid():N}.txt");
        try
        {
            var command = new SubscribeCommand(factory)
            {
                Url = "opc.tcp://localhost:4840",
                NodeId = "ns=2;s=SummaryFileNode",
                DurationSeconds = 1,
                SummaryFile = tempFile
            };
            using var console = TestConsoleHelper.CreateConsole();
            await command.ExecuteAsync(console);
            File.Exists(tempFile).ShouldBeTrue();
            var contents = await File.ReadAllTextAsync(tempFile);
            contents.ShouldContain("SUMMARY");
            contents.ShouldContain("Total subscribed: 1");
        }
        finally
        {
            if (File.Exists(tempFile)) File.Delete(tempFile);
        }
    }
    [Fact]
    public async Task Recursive_BrowsesSubtreeAndSubscribesEveryVariable()
    {
        // Root has a Folder child and a top-level Variable; the Folder contains a second Variable.
        // Each node id is distinct so the ToDictionary(t => t.nodeId.ToString()) collapse
        // doesn't trip a duplicate-key error.
        var fakeService = new FakeOpcUaClientService();
        fakeService.BrowseResultsByParent["ns=2;s=Root"] = new List<BrowseResult>
        {
            new("ns=2;s=Folder", "Folder", "Object", true),
            new("ns=2;s=Tag1", "Tag1", "Variable", false)
        };
        fakeService.BrowseResultsByParent["ns=2;s=Folder"] = new List<BrowseResult>
        {
            new("ns=2;s=Tag2", "Tag2", "Variable", false)
        };
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=Root",
            Recursive = true,
            MaxDepth = 3,
            DurationSeconds = 1
        };
        using var console = TestConsoleHelper.CreateConsole();
        await command.ExecuteAsync(console);
        // Both Variables (depth 1 + depth 2) should have been subscribed.
        fakeService.SubscribeCalls.Count.ShouldBeGreaterThanOrEqualTo(2);
        fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag1");
        fakeService.SubscribeCalls.ShouldContain(c => c.NodeId.Identifier.ToString() == "Tag2");
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("Browsing subtree of ns=2;s=Root");
    }
    [Fact]
    public async Task SubscribeFailure_PrintsFailedMessage_DoesNotCrash()
    {
        var fakeService = new FakeOpcUaClientService
        {
            SubscribeException = new InvalidOperationException("forced failure")
        };
        var factory = new FakeOpcUaClientServiceFactory(fakeService);
        var command = new SubscribeCommand(factory)
        {
            Url = "opc.tcp://localhost:4840",
            NodeId = "ns=2;s=FailNode",
            DurationSeconds = 1
        };
        using var console = TestConsoleHelper.CreateConsole();
        await command.ExecuteAsync(console);
        var output = TestConsoleHelper.GetOutput(console);
        output.ShouldContain("FAILED to subscribe");
        output.ShouldContain("forced failure");
        // The summary block is still printed.
        output.ShouldContain("==================== SUMMARY ====================");
    }
    private static async Task WaitForAsync(Func<bool> predicate, int timeoutMs = 5000)
    {
        var deadline = DateTime.UtcNow.AddMilliseconds(timeoutMs);
        while (!predicate() && DateTime.UtcNow < deadline)
            await Task.Delay(25);
        if (!predicate())
            throw new TimeoutException("Condition not met within timeout.");
    }
 }
@@ -0,0 +1,163 @@
 using Opc.Ua;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.Shared.Adapters;
 namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests.Adapters;
 /// <summary>
 /// Regression tests for the pure best-endpoint selection logic extracted from
 /// <see cref="DefaultEndpointDiscovery"/> (Client.Shared-011). The selector picks the best
 /// endpoint matching the requested security mode (preferring Basic256Sha256) and rewrites
 /// the discovered endpoint URL hostname to match the operator-supplied URL so internal DNS
 /// hostnames in discovery responses do not leak into the session.
 /// </summary>
 public class EndpointSelectorTests
 {
    private static EndpointDescription Ep(MessageSecurityMode mode, string policy, string url)
    {
        return new EndpointDescription
        {
            EndpointUrl = url,
            SecurityMode = mode,
            SecurityPolicyUri = policy,
        };
    }
    /// <summary>
    /// Verifies that the selector returns the only endpoint matching the requested
    /// security mode even when other endpoints with different modes are present.
    /// </summary>
    [Fact]
    public void SelectBest_PicksMatchingSecurityMode()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"),
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
            Ep(MessageSecurityMode.SignAndEncrypt, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
        };
        var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
        best.SecurityMode.ShouldBe(MessageSecurityMode.Sign);
        best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256);
    }
    /// <summary>
    /// Verifies that when multiple endpoints match the requested mode, Basic256Sha256 wins
    /// over weaker policies — even when Basic256Sha256 is not the first encountered.
    /// </summary>
    [Fact]
    public void SelectBest_PrefersBasic256Sha256WhenMultipleMatch()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"),
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256, "opc.tcp://server:4840"),
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"),
        };
        var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
        best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic256Sha256);
    }
    /// <summary>
    /// Verifies that the selector falls back to the first matching endpoint when no
    /// Basic256Sha256 endpoint is advertised for the requested security mode.
    /// </summary>
    [Fact]
    public void SelectBest_FallsBackToFirstMatchWhenNoBasic256Sha256()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic128Rsa15, "opc.tcp://server:4840"),
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256, "opc.tcp://server:4840"),
        };
        var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.Sign);
        best.SecurityPolicyUri.ShouldBe(SecurityPolicies.Basic128Rsa15);
    }
    /// <summary>
    /// Verifies that no matching endpoint produces an InvalidOperationException whose
    /// message lists the available security mode/policy combinations to aid diagnosis.
    /// </summary>
    [Fact]
    public void SelectBest_NoMatchingMode_ThrowsWithDiagnostic()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.None, SecurityPolicies.None, "opc.tcp://server:4840"),
        };
        var ex = Should.Throw<InvalidOperationException>(() =>
            EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840", MessageSecurityMode.SignAndEncrypt));
        ex.Message.ShouldContain("SignAndEncrypt");
        ex.Message.ShouldContain("None"); // available endpoint listed in the message
    }
    /// <summary>
    /// Verifies that the selector rewrites the discovery-returned hostname to the
    /// operator-supplied hostname so internal DNS names in the response do not leak
    /// into the resulting session.
    /// </summary>
    [Fact]
    public void SelectBest_RewritesHostToMatchRequestedUrl()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256,
                "opc.tcp://internal-host:4840/UA/Server"),
        };
        var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://external-host:4840",
            MessageSecurityMode.Sign);
        new Uri(best.EndpointUrl).Host.ShouldBe("external-host");
    }
    /// <summary>
    /// Verifies that when the discovery host already matches the requested host the
    /// endpoint URL is left untouched.
    /// </summary>
    [Fact]
    public void SelectBest_HostsMatch_LeavesUrlUnchanged()
    {
        var endpoints = new[]
        {
            Ep(MessageSecurityMode.Sign, SecurityPolicies.Basic256Sha256,
                "opc.tcp://server:4840/UA/Server"),
        };
        var best = EndpointSelector.SelectBest(endpoints, "opc.tcp://server:4840",
            MessageSecurityMode.Sign);
        best.EndpointUrl.ShouldBe("opc.tcp://server:4840/UA/Server");
    }
    /// <summary>
    /// Verifies that a null endpoints argument throws ArgumentNullException rather than
    /// producing a confusing downstream NullReferenceException.
    /// </summary>
    [Fact]
    public void SelectBest_NullEndpoints_Throws()
    {
        Should.Throw<ArgumentNullException>(() =>
            EndpointSelector.SelectBest(null!, "opc.tcp://server:4840", MessageSecurityMode.None));
    }
    /// <summary>
    /// Verifies that an empty endpointUrl produces ArgumentException so the caller gets
    /// a clear contract violation rather than a downstream UriFormatException.
    /// </summary>
    [Fact]
    public void SelectBest_EmptyEndpointUrl_Throws()
    {
        Should.Throw<ArgumentException>(() =>
            EndpointSelector.SelectBest(Array.Empty<EndpointDescription>(), "", MessageSecurityMode.None));
    }
 }
@@ -168,10 +168,25 @@ internal sealed class FakeSessionAdapter : ISessionAdapter
        }
    }
    /// <summary>
    /// Number of times <see cref="CallMethodAsync"/> was invoked so tests can assert
    /// acknowledgment workflows reached the session adapter.
    /// </summary>
    public int CallMethodCount { get; private set; }
    /// <summary>
    /// When set, <see cref="CallMethodAsync"/> throws this exception — used to simulate
    /// a bad method call status surfacing as a <see cref="ServiceResultException"/>.
    /// </summary>
    public Exception? CallMethodException { get; set; }
    /// <inheritdoc />
    public Task<IList<object>?> CallMethodAsync(NodeId objectId, NodeId methodId, object[] inputArguments,
        CancellationToken ct = default)
    {
        CallMethodCount++;
        if (CallMethodException != null)
            throw CallMethodException;
        return Task.FromResult<IList<object>?>(null);
    }
@@ -18,8 +18,10 @@ public class ConnectionSettingsTests
        settings.SecurityMode.ShouldBe(SecurityMode.None);
        settings.SessionTimeoutSeconds.ShouldBe(60);
        settings.AutoAcceptCertificates.ShouldBeTrue();
-        settings.CertificateStorePath.ShouldContain("OtOpcUaClient");
+        // CertificateStorePath defaults to empty so constructing settings does not
-        settings.CertificateStorePath.ShouldContain("pki");
+        // touch disk; DefaultApplicationConfigurationFactory resolves the canonical
        // PKI path lazily on first connect (Client.Shared-010).
        settings.CertificateStorePath.ShouldBe(string.Empty);
    }
    [Fact]
@@ -996,6 +996,143 @@ public class OpcUaClientServiceTests : IDisposable
        eventCount.ShouldBe(0);
    }
    // --- AcknowledgeAlarm tests (Client.Shared-009) ---
    /// <summary>
    /// Verifies that a successful acknowledge call returns <see cref="StatusCodes.Good"/>
    /// and reaches the session adapter's CallMethodAsync (Client.Shared-009).
    /// </summary>
    [Fact]
    public async Task AcknowledgeAlarmAsync_OnSuccess_ReturnsGood()
    {
        var session = new FakeSessionAdapter();
        _sessionFactory.EnqueueSession(session);
        await _service.ConnectAsync(ValidSettings());
        var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked");
        result.ShouldBe(StatusCodes.Good);
        session.CallMethodCount.ShouldBe(1);
    }
    /// <summary>
    /// Regression for Client.Shared-009: a bad call result must surface as the returned
    /// <see cref="StatusCode"/> rather than escape as an uncaught
    /// <see cref="ServiceResultException"/>, so callers using
    /// <c>if (StatusCode.IsBad(result))</c> actually see the failure.
    /// </summary>
    [Fact]
    public async Task AcknowledgeAlarmAsync_OnServiceResultException_ReturnsBadStatusCode()
    {
        var session = new FakeSessionAdapter
        {
            CallMethodException = new ServiceResultException(
                StatusCodes.BadConditionAlreadyEnabled, "already acked")
        };
        _sessionFactory.EnqueueSession(session);
        await _service.ConnectAsync(ValidSettings());
        var result = await _service.AcknowledgeAlarmAsync("ns=2;s=Cond", new byte[] { 1, 2 }, "acked");
        StatusCode.IsBad(result).ShouldBeTrue();
        result.Code.ShouldBe(StatusCodes.BadConditionAlreadyEnabled);
    }
    /// <summary>
    /// Verifies the ".Condition" suffix is appended when the caller supplies the
    /// source node, but left alone when the caller already passes the condition node —
    /// matches the documented contract.
    /// </summary>
    [Fact]
    public async Task AcknowledgeAlarmAsync_LeavesConditionSuffixAlone()
    {
        var session = new FakeSessionAdapter();
        _sessionFactory.EnqueueSession(session);
        await _service.ConnectAsync(ValidSettings());
        await _service.AcknowledgeAlarmAsync("ns=2;s=Cond.Condition", new byte[] { 1, 2 }, "acked");
        // Both call shapes reach the adapter once.
        session.CallMethodCount.ShouldBe(1);
    }
    // --- Alarm fallback path (Client.Shared-011) ---
    /// <summary>
    /// Regression for Client.Shared-011: when standard AckedState/Id and ActiveState/Id
    /// fields are missing (null Value) but the SourceNode (ConditionId) field at index 12
    /// is populated, the client launches the Task.Run fallback that reads
    /// <c>InAlarm</c>/<c>Acked</c> from the condition node's Galaxy attributes. Verify
    /// the alarm event is delivered with the values from the supplemental reads.
    /// </summary>
    [Fact]
    public async Task OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent()
    {
        var fakeSub = new FakeSubscriptionAdapter();
        var session = new FakeSessionAdapter
        {
            NextSubscription = fakeSub,
            ReadResponseFunc = nodeId =>
            {
                var key = nodeId.ToString();
                if (key.EndsWith(".InAlarm"))
                    return new DataValue(new Variant(true), StatusCodes.Good);
                if (key.EndsWith(".Acked"))
                    return new DataValue(new Variant(false), StatusCodes.Good);
                if (key.EndsWith(".TimeAlarmOn"))
                    return new DataValue(new Variant(new DateTime(2026, 1, 1, 12, 0, 0)), StatusCodes.Good);
                if (key.EndsWith(".DescAttrName"))
                    return new DataValue(new Variant("Fallback message"), StatusCodes.Good);
                return new DataValue(StatusCodes.BadNodeIdUnknown);
            }
        };
        _sessionFactory.EnqueueSession(session);
        await _service.ConnectAsync(ValidSettings());
        AlarmEventArgs? received = null;
        var raised = new TaskCompletionSource();
        _service.AlarmEvent += (_, e) =>
        {
            received = e;
            raised.TrySetResult();
        };
        await _service.SubscribeAlarmsAsync();
        var handle = fakeSub.ActiveHandles.First();
        // AckedState/Id (8) and ActiveState/Id (9) are present but Variant.Value is null,
        // which triggers the supplemental Galaxy-attribute fallback; SourceNode (12) is set.
        var fields = new EventFieldList
        {
            EventFields =
            [
                new Variant(new byte[] { 1, 2, 3 }),       // 0: EventId
                new Variant(ObjectTypeIds.AlarmConditionType), // 1: EventType
                new Variant("Source1"),                    // 2: SourceName
                new Variant(DateTime.MinValue),            // 3: Time
                new Variant(new LocalizedText("Initial")), // 4: Message
                new Variant((ushort)400),                  // 5: Severity
                new Variant("CondName"),                   // 6: ConditionName
                new Variant(true),                         // 7: Retain
                Variant.Null,                              // 8: AckedState/Id — missing
                Variant.Null,                              // 9: ActiveState/Id — missing
                new Variant(true),                         // 10: EnabledState/Id
                new Variant(false),                        // 11: SuppressedOrShelved
                new Variant("ns=2;s=ConditionId")          // 12: SourceNode
            ]
        };
        fakeSub.SimulateEvent(handle, fields);
        // The fallback runs on a background Task.Run continuation — wait briefly for it.
        await Task.WhenAny(raised.Task, Task.Delay(500));
        received.ShouldNotBeNull();
        received!.ActiveState.ShouldBeTrue();  // from InAlarm read
        received.AckedState.ShouldBeFalse();   // from Acked read
        received.ConditionNodeId.ShouldBe("ns=2;s=ConditionId");
        received.Message.ShouldBe("Fallback message"); // from DescAttrName read
    }
    // --- Failover tests ---
    /// <summary>
@@ -0,0 +1,61 @@
 using Shouldly;
 using Xunit;
 namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
 /// <summary>
 ///     Driver.FOCAS.Cli-004: every FOCAS CLI command must own one disposal mechanism for
 ///     the <c>FocasDriver</c>, not two. The chosen mechanism is <c>await using var driver
 ///     = ...</c> — <c>FocasDriver.DisposeAsync</c> already calls <c>ShutdownAsync</c>, so
 ///     an additional explicit <c>driver.ShutdownAsync(...)</c> in a <c>finally</c> block
 ///     runs shutdown twice. 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_FocasDriver(string commandFile)
    {
        var path = Path.Combine(CommandsDir, commandFile);
        var source = File.ReadAllText(path);
        source.ShouldContain("await using var driver = new FocasDriver(");
    }
    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.FOCAS.Cli", "Commands");
    }
 }
@@ -0,0 +1,86 @@
 using CliFx.Attributes;
 using CliFx.Infrastructure;
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
 namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
 /// <summary>
 ///     Covers <see cref="FocasCommandBase.BuildOptions"/> — the pure, deterministic mapping
 ///     from the base's CNC host/port/series/timeout flags onto a
 ///     <c>FocasDriverOptions</c>. The CLI is one-shot so the background connectivity probe
 ///     must be disabled.
 /// </summary>
 [Trait("Category", "Unit")]
 public sealed class FocasCommandBaseBuildOptionsTests
 {
    [Command("noop-test", Description = "Test-only probe of FocasCommandBase.BuildOptions.")]
    private sealed class ProbeOnly : FocasCommandBase
    {
        public override ValueTask ExecuteAsync(IConsole console) => default;
        public FocasDriverOptions Invoke(IReadOnlyList<FocasTagDefinition> tags) => BuildOptions(tags);
    }
    [Fact]
    public void BuildOptions_disables_probe_for_one_shot_cli_runs()
    {
        var sut = new ProbeOnly
        {
            CncHost = "10.0.0.5",
            CncPort = 8193,
            Series = FocasCncSeries.ThirtyOne_i,
            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 { CncHost = "h", TimeoutMs = 7500 };
        var options = sut.Invoke([]);
        options.Timeout.ShouldBe(TimeSpan.FromMilliseconds(7500));
    }
    [Fact]
    public void BuildOptions_flows_host_port_series_through()
    {
        var sut = new ProbeOnly
        {
            CncHost = "cnc.shop.local",
            CncPort = 8194,
            Series = FocasCncSeries.Zero_i_F,
            TimeoutMs = 3000,
        };
        var options = sut.Invoke([]);
        options.Devices.Count.ShouldBe(1);
        options.Devices[0].HostAddress.ShouldBe("focas://cnc.shop.local:8194");
        options.Devices[0].Series.ShouldBe(FocasCncSeries.Zero_i_F);
    }
    [Fact]
    public void BuildOptions_forwards_tag_list_verbatim()
    {
        var sut = new ProbeOnly { CncHost = "h" };
        var tag = new FocasTagDefinition(
            Name: "t",
            DeviceHostAddress: "focas://h:8193",
            Address: "R100",
            DataType: FocasDataType.Int16,
            Writable: false);
        var options = sut.Invoke([tag]);
        options.Tags.Count.ShouldBe(1);
        options.Tags[0].ShouldBeSameAs(tag);
    }
 }
@@ -0,0 +1,79 @@
 using CliFx.Attributes;
 using CliFx.Infrastructure;
 using Shouldly;
 using Xunit;
 namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
 /// <summary>
 ///     Driver.FOCAS.Cli-003: numeric options that are not range-checked at the CLI
 ///     boundary surface either as opaque downstream exceptions or as tight-spinning
 ///     poll loops rather than a clear "value must be positive" message. These tests
 ///     pin the validation contract for <c>--cnc-port</c>, <c>--timeout-ms</c>, and
 ///     <c>--interval-ms</c>.
 /// </summary>
 [Trait("Category", "Unit")]
 public sealed class FocasCommandBaseValidationTests
 {
    // Test-only FocasCommandBase concrete subclass that exposes the protected ValidateOptions
    // 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 FocasCommandBase.ValidateOptions.")]
    private sealed class Probe : FocasCommandBase
    {
        public int IntervalMs { get; init; }
        public override ValueTask ExecuteAsync(IConsole console) => default;
        public void InvokeValidate() => ValidateOptions(IntervalMs);
        public void InvokeValidateNoInterval() => ValidateOptions(intervalMs: null);
    }
    [Fact]
    public void Validate_accepts_default_options()
    {
        var sut = new Probe { CncHost = "host", IntervalMs = 1000 };
        Should.NotThrow(() => sut.InvokeValidate());
    }
    [Theory]
    [InlineData(0)]
    [InlineData(-1)]
    [InlineData(65536)]
    public void Validate_rejects_out_of_range_cnc_port(int port)
    {
        var sut = new Probe { CncHost = "host", CncPort = port, IntervalMs = 1000 };
        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
        ex.Message.ShouldContain("cnc-port", Case.Insensitive);
    }
    [Theory]
    [InlineData(0)]
    [InlineData(-100)]
    public void Validate_rejects_non_positive_timeout_ms(int timeoutMs)
    {
        var sut = new Probe { CncHost = "host", TimeoutMs = timeoutMs, IntervalMs = 1000 };
        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
        ex.Message.ShouldContain("timeout-ms", Case.Insensitive);
    }
    [Theory]
    [InlineData(0)]
    [InlineData(-500)]
    public void Validate_rejects_non_positive_interval_ms(int intervalMs)
    {
        var sut = new Probe { CncHost = "host", IntervalMs = intervalMs };
        var ex = Should.Throw<CliFx.Exceptions.CommandException>(() => sut.InvokeValidate());
        ex.Message.ShouldContain("interval-ms", Case.Insensitive);
    }
    [Fact]
    public void Validate_skips_interval_check_when_command_omits_it()
    {
        // probe / read / write don't take an --interval-ms option; the validator must
        // skip that check when the caller passes null.
        var sut = new Probe { CncHost = "host" };
        Should.NotThrow(() => sut.InvokeValidateNoInterval());
    }
 }
@@ -0,0 +1,55 @@
 using Shouldly;
 using Xunit;
 namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
 /// <summary>
 ///     Driver.FOCAS.Cli-002: the FOCAS <c>subscribe</c> command — a near-verbatim copy
 ///     of the Modbus subscribe command — must
 ///     (a) serialise writes from the <c>OnDataChange</c> handler (raised from the
 ///         driver's <c>PollGroupEngine</c> background thread) with a lock, so the
 ///         "Subscribed to ..." banner write from the CliFx main thread cannot interleave
 ///         with the first poll-driven change line; and
 ///     (b) carry the explanatory comment that documents 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 FOCAS
 ///         copy must too.
 /// </summary>
 [Trait("Category", "Unit")]
 public sealed class SubscribeCommandConsoleHandlerTests
 {
    private static string ReadSubscribeSource()
    {
        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();
        return File.ReadAllText(Path.Combine(
            dir!.FullName, "src", "Drivers", "Cli", "ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli",
            "Commands", "SubscribeCommand.cs"));
    }
    [Fact]
    public void SubscribeCommand_explains_why_OnDataChange_uses_console_Output_synchronously()
    {
        var source = ReadSubscribeSource();
        // The comment must reference the CliFx console abstraction so future copy-pastes
        // do not lose the rationale.
        source.ShouldContain("CliFx console");
        source.ShouldContain("IConsole");
    }
    [Fact]
    public void SubscribeCommand_serialises_console_writes_with_a_lock()
    {
        var source = ReadSubscribeSource();
        // Both the banner write and the OnDataChange handler must share a writeLock so the
        // banner from the CliFx invocation thread cannot interleave with the first
        // poll-driven change line from the driver tick thread.
        source.ShouldContain("writeLock");
        source.ShouldContain("lock (writeLock)");
    }
 }
@@ -0,0 +1,122 @@
 using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
 using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Commands;
 namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests;
 /// <summary>
 ///     Covers <see cref="WriteCommand.ParseValue"/> across every FOCAS atomic type.
 ///     Driver.FOCAS.Cli-001: malformed numeric input must surface as a friendly
 ///     <see cref="CliFx.Exceptions.CommandException"/>, not a raw
 ///     <see cref="FormatException"/> / <see cref="OverflowException"/> stack trace.
 /// </summary>
 [Trait("Category", "Unit")]
 public sealed class WriteCommandParseValueTests
 {
    [Theory]
    [InlineData("true", true)]
    [InlineData("0", false)]
    [InlineData("yes", true)]
    [InlineData("OFF", false)]
    public void ParseValue_Bit_accepts_common_boolean_aliases(string raw, bool expected)
    {
        WriteCommand.ParseValue(raw, FocasDataType.Bit).ShouldBe(expected);
    }
    [Fact]
    public void ParseValue_Bit_rejects_garbage_as_CommandException()
    {
        Should.Throw<CliFx.Exceptions.CommandException>(
            () => WriteCommand.ParseValue("maybe", FocasDataType.Bit));
    }
    [Fact]
    public void ParseValue_Byte_signed_range()
    {
        // FocasDataType.Byte is signed (PMC byte read returns int8).
        WriteCommand.ParseValue("-128", FocasDataType.Byte).ShouldBe((sbyte)-128);
        WriteCommand.ParseValue("127", FocasDataType.Byte).ShouldBe((sbyte)127);
    }
    [Fact]
    public void ParseValue_Int16_signed_range()
    {
        WriteCommand.ParseValue("-32768", FocasDataType.Int16).ShouldBe(short.MinValue);
        WriteCommand.ParseValue("32767", FocasDataType.Int16).ShouldBe(short.MaxValue);
    }
    [Fact]
    public void ParseValue_Int32_parses_negative()
    {
        WriteCommand.ParseValue("-2147483648", FocasDataType.Int32).ShouldBe(int.MinValue);
    }
    [Fact]
    public void ParseValue_Float32_invariant_culture()
    {
        WriteCommand.ParseValue("3.14", FocasDataType.Float32).ShouldBe(3.14f);
    }
    [Fact]
    public void ParseValue_Float64_higher_precision()
    {
        WriteCommand.ParseValue("2.718281828", FocasDataType.Float64).ShouldBeOfType<double>();
    }
    [Fact]
    public void ParseValue_String_passthrough()
    {
        WriteCommand.ParseValue("hello fanuc", FocasDataType.String).ShouldBe("hello fanuc");
    }
    // Driver.FOCAS.Cli-001: malformed input must produce a CommandException (a clean
    // one-line CliFx error), NOT a raw FormatException stack trace. Previously the raw
    // BCL parser exceptions leaked, contradicting how the Bit path already handled bad
    // boolean input.
    [Theory]
    [InlineData("xyz", FocasDataType.Byte)]
    [InlineData("xyz", FocasDataType.Int16)]
    [InlineData("xyz", FocasDataType.Int32)]
    [InlineData("not-a-number", FocasDataType.Float32)]
    [InlineData("also-bad", FocasDataType.Float64)]
    public void ParseValue_non_numeric_for_numeric_types_throws_CommandException(
        string raw, FocasDataType type)
    {
        Should.Throw<CliFx.Exceptions.CommandException>(
            () => WriteCommand.ParseValue(raw, type));
    }
    // OverflowException from out-of-range input must also surface as CommandException.
    [Theory]
    [InlineData("128", FocasDataType.Byte)]        // sbyte max + 1
    [InlineData("-129", FocasDataType.Byte)]       // sbyte min - 1
    [InlineData("32768", FocasDataType.Int16)]     // short max + 1
    [InlineData("9999999999", FocasDataType.Int32)] // > int max
    public void ParseValue_overflow_for_numeric_types_throws_CommandException(
        string raw, FocasDataType type)
    {
        Should.Throw<CliFx.Exceptions.CommandException>(
            () => WriteCommand.ParseValue(raw, type));
    }
    [Fact]
    public void ParseValue_CommandException_message_names_the_type_and_value()
    {
        var ex = Should.Throw<CliFx.Exceptions.CommandException>(
            () => WriteCommand.ParseValue("xyz", FocasDataType.Int16));
        ex.Message.ShouldContain("xyz");
        ex.Message.ShouldContain("Int16");
    }
    [Theory]
    [InlineData("R100", FocasDataType.Int16, "R100:Int16")]
    [InlineData("X0.0", FocasDataType.Bit, "X0.0:Bit")]
    [InlineData("PARAM:1815/0", FocasDataType.Int32, "PARAM:1815/0:Int32")]
    [InlineData("MACRO:500", FocasDataType.Float64, "MACRO:500:Float64")]
    public void SynthesiseTagName_preserves_FOCAS_address_verbatim(
        string address, FocasDataType type, string expected)
    {
        ReadCommand.SynthesiseTagName(address, type).ShouldBe(expected);
    }
 }
@@ -0,0 +1,26 @@
 <Project Sdk="Microsoft.NET.Sdk">
    <PropertyGroup>
        <TargetFramework>net10.0</TargetFramework>
        <Nullable>enable</Nullable>
        <ImplicitUsings>enable</ImplicitUsings>
        <IsPackable>false</IsPackable>
        <IsTestProject>true</IsTestProject>
        <RootNamespace>ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests</RootNamespace>
    </PropertyGroup>
    <ItemGroup>
        <PackageReference Include="xunit.v3" Version="1.1.0"/>
        <PackageReference Include="Shouldly" Version="4.3.0"/>
        <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0"/>
        <PackageReference Include="xunit.runner.visualstudio" Version="3.0.2">
            <PrivateAssets>all</PrivateAssets>
            <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
        </PackageReference>
    </ItemGroup>
    <ItemGroup>
        <ProjectReference Include="..\..\..\..\src\Drivers\Cli\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli\ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.csproj"/>
    </ItemGroup>
 </Project>
@@ -491,4 +491,95 @@ public sealed class WonderwareHistorianClientTests
        await Should.ThrowAsync<InvalidDataException>(() =>
            client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 100, CancellationToken.None));
    }
    // ===== Finding-003 / Finding-004: health counter consistency =====
    /// <summary>
    ///     (Finding 003 + 004) A sidecar-level failure must be classified once: TotalSuccesses
    ///     must stay at 0, TotalFailures must become 1, and TotalQueries / TotalSuccesses /
    ///     TotalFailures must all be updated under the same lock so a concurrent snapshot can
    ///     never observe inflated successes or out-of-band TotalQueries. This pins behaviour so
    ///     a future regression to the "RecordSuccess then undo via ReclassifySuccessAsFailure"
    ///     dance is caught.
    /// </summary>
    [Fact]
    public async Task GetHealthSnapshot_SidecarFailure_NeverInflatesSuccessCounter()
    {
        var pipe = UniquePipeName();
        await using var server = new FakeSidecarServer(pipe, Secret)
        {
            OnReadRaw = _ => new ReadRawReply { Success = false, Error = "boom" },
        };
        await server.StartAsync();
        await using var client = new WonderwareHistorianClient(OptsFor(pipe));
        await Should.ThrowAsync<InvalidOperationException>(() =>
            client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 1, CancellationToken.None));
        var snap = client.GetHealthSnapshot();
        snap.TotalQueries.ShouldBe(1);
        snap.TotalSuccesses.ShouldBe(0);
        snap.TotalFailures.ShouldBe(1);
        snap.ConsecutiveFailures.ShouldBe(1);
        snap.LastError.ShouldNotBeNull();
    }
    /// <summary>
    ///     (Finding 003) Concurrent calls + concurrent <see cref="WonderwareHistorianClient.GetHealthSnapshot"/>
    ///     reads must observe consistent counters. Specifically, TotalSuccesses + TotalFailures
    ///     must equal TotalQueries at every observed snapshot (no torn read between an
    ///     Interlocked-incremented TotalQueries and a lock-protected outcome counter). The
    ///     channel serializes calls, so the test is observable: each completed query strictly
    ///     increments either successes or failures by one.
    /// </summary>
    [Fact]
    public async Task GetHealthSnapshot_ConcurrentCallsAndReads_CountersAreInternallyConsistent()
    {
        var pipe = UniquePipeName();
        await using var server = new FakeSidecarServer(pipe, Secret)
        {
            OnReadRaw = _ => new ReadRawReply { Success = true },
        };
        await server.StartAsync();
        await using var client = new WonderwareHistorianClient(OptsFor(pipe));
        using var stop = new CancellationTokenSource();
        var readerSawInconsistent = false;
 #pragma warning disable xUnit1051 // Internal Task.Run loop drives a polling stress test; cancellation flows via stop.IsCancellationRequested below.
        var reader = Task.Run(() =>
        {
            while (!stop.IsCancellationRequested)
            {
                var snap = client.GetHealthSnapshot();
                // Every completed call increments TotalQueries AND exactly one of
                // TotalSuccesses or TotalFailures under the same lock; an in-flight call
                // has not yet incremented any of them. So TotalQueries should always equal
                // the sum of TotalSuccesses + TotalFailures (no in-between state visible).
                if (snap.TotalSuccesses + snap.TotalFailures != snap.TotalQueries)
                {
                    readerSawInconsistent = true;
                }
            }
        });
 #pragma warning restore xUnit1051
        for (var i = 0; i < 50; i++)
        {
            await client.ReadRawAsync("Tag", DateTime.UtcNow, DateTime.UtcNow, 1, TestContext.Current.CancellationToken);
        }
        stop.Cancel();
        await reader;
        readerSawInconsistent.ShouldBeFalse(
            "GetHealthSnapshot exposed TotalQueries that disagreed with the sum of TotalSuccesses + TotalFailures — counters are not updated under a single lock.");
        var final = client.GetHealthSnapshot();
        final.TotalQueries.ShouldBe(50);
        final.TotalSuccesses.ShouldBe(50);
        final.TotalFailures.ShouldBe(0);
    }
 }
Author	SHA1	Message	Date
Joseph Doherty	59ecd18169	docs(code-reviews): regenerate index — 25 Low findings resolved Batch 6 cleared Open findings in Driver.FOCAS.Cli (1 deferred to Driver.Cli.Common), Driver.Cli.Common, Driver.Historian.Wonderware.Client, Client.CLI, and Client.Shared. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:13:29 -04:00
Joseph Doherty	2a6ac07111	fix(client-shared): resolve Low code-review findings (Client.Shared-003,004,009,010,011) - Client.Shared-003: DefaultSessionAdapter.WriteValueAsync / CallMethodAsync guard against null/empty Results and throw ServiceResultException with the response's ServiceResult code instead of indexing into a missing list. - Client.Shared-004: DefaultSessionAdapter.CloseAsync / HistoryReadRawAsync / HistoryReadAggregateAsync use the Session.*Async overloads and honour the caller's CancellationToken. - Client.Shared-009: AcknowledgeAlarmAsync returns the underlying ServiceResultException.StatusCode on failure instead of always Good; IOpcUaClientService doc updated to describe the new contract. - Client.Shared-010: ConnectionSettings.CertificateStorePath defaults to empty; DefaultApplicationConfigurationFactory resolves the canonical PKI path lazily, so per-failover ConnectionSettings copies don't hit the filesystem. - Client.Shared-011: added the alarm-fallback regression test, extracted EndpointSelector as a pure static, and added EndpointSelectorTests covering security-mode match, Basic256Sha256 preference, fallback, diagnostics, hostname rewrite, and null/empty guards. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:13:21 -04:00
Joseph Doherty	7fe9f16cf8	fix(client-cli): resolve Low code-review findings (Client.CLI-002,003,004,006,007,008,009,010) - Client.CLI-002: SubscribeCommand's neverWentBad list now requires the node to be present in lastStatus (i.e. received at least one update) so the 'suspect' bucket only contains observed nodes. - Client.CLI-003: every long-running command validates numeric option ranges (Interval / Depth / MaxDepth / Duration / Max) and throws CliFx CommandException on out-of-range values. - Client.CLI-004: SubscribeCommand carries XML summary docs on the type, ctor, every [CommandOption] property, and ExecuteAsync — matching the sibling commands' style. - Client.CLI-006: HistoryReadCommand parses --start / --end with InvariantCulture+UTC and surfaces FormatException as CommandException; every NodeIdParser.ParseRequired call wraps FormatException / ArgumentException as CommandException. - Client.CLI-007: CommandBase.ConfigureLogging calls Log.CloseAndFlush() before assigning a new Log.Logger so prior sinks are disposed. - Client.CLI-008: rewrote the subscribe and historyread sections of docs/Client.CLI.md (every flag documented, summary-bucket vocabulary, StandardDeviation aggregate, UTC --start/--end convention). - Client.CLI-009: SubscribeCommand / AlarmsCommand use named local handlers and detach them via -= after UnsubscribeAsync so no notification reaches the console after the command's output phase ends. - Client.CLI-010: added CommandRangeValidationTests, EventHandlerLifecycleTests, InputValidationErrorsTests, LoggerLifecycleTests, and SubscribeCommandSummaryTests pinning every Low fix; FakeOpcUaClientService gained AddDiscoveredVariable + RaiseDataChanged + BrowseResultsByParent helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:13:08 -04:00
Joseph Doherty	879925180b	fix(driver-historian-wonderware-client): resolve Low code-review findings (Driver.Historian.Wonderware.Client-003,004,006,008,010) - Driver.Historian.Wonderware.Client-003: replaced the mixed Interlocked + healthLock counters with RecordOutcome that touches _totalQueries and exactly one of _totalSuccesses / _totalFailures under one acquisition. - Driver.Historian.Wonderware.Client-004: InvokeAndClassifyAsync routes transport + sidecar classification through a single RecordOutcome call; the legacy ReclassifySuccessAsFailure two-step is gone. - Driver.Historian.Wonderware.Client-006: removed the dead ReconnectInitialBackoff / ReconnectMaxBackoff options and added a doc <remarks> stating the channel performs a single in-place reconnect; retry/backoff stays with the caller. - Driver.Historian.Wonderware.Client-008: the audit-suppression comment block now records advisory titles, why neither applies, and the revisit trigger. - Driver.Historian.Wonderware.Client-010: reworded Dispose() to claim deadlock-safety and added a GetHealthSnapshot summary documenting the single-channel collapse + counter invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:12:16 -04:00
Joseph Doherty	3ca569f621	fix(driver-cli-common): resolve Low code-review findings (Driver.Cli.Common-004,006) - Driver.Cli.Common-004: confirm the FormatTable empty-input guard landed earlier (commit `1433a1c`); flip status to Resolved with a cross-reference. - Driver.Cli.Common-006: reword the SnapshotFormatter source-time column comment to describe the actual behaviour (right-most column, unmeasured, '-' for null timestamps) and confirm the DriverCommandBase summary now enumerates FOCAS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:12:04 -04:00
Joseph Doherty	6923be3aa2	fix(driver-focas-cli): resolve Low code-review findings (Driver.FOCAS.Cli-001,002,003,004; -005 deferred) - Driver.FOCAS.Cli-001: WriteCommand.ParseValue now wraps numeric FormatException / OverflowException as CliFx CommandException with the offending value. - Driver.FOCAS.Cli-002: SubscribeCommand's OnDataChange handler and the banner both take a writeLock so notification-callback and main-thread writes can't interleave; handler exceptions are warn-and-swallow. - Driver.FOCAS.Cli-003: FocasCommandBase.ValidateOptions rejects --cnc-port outside 1..65535, non-positive --timeout-ms, and non-positive --interval-ms; ExecuteAsync calls it first. - Driver.FOCAS.Cli-004: 'await using var driver' is the sole driver disposal path; dropped the redundant explicit await ShutdownAsync. - Driver.FOCAS.Cli-005 (Deferred): the fix lives in Driver.Cli.Common.SnapshotFormatter — explicitly naming the status-code shortlist there benefits every driver CLI. Left as a Driver.Cli.Common follow-up. - Registered the new tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Cli.Tests project in ZB.MOM.WW.OtOpcUa.slnx. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 11:11:55 -04:00