6 Commits

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
48 changed files with 2213 additions and 259 deletions
+1
View File
@@ -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" />
+17 -17
View File
@@ -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.
+11 -11
View File
@@ -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.
+15 -5
View File
@@ -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).
+55 -11
View File
@@ -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`.
+30 -30
View File
@@ -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.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 5 | 11 |
| [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 | 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` |
+40 -16
View File
@@ -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) |
| `-i` / `--interval` | Sampling/publishing interval in milliseconds (default: 1000) |
| `-n` / `--node` | Node ID to monitor (required). When `--recursive` is set, this is the browse root. |
| `-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) |
| `--end` | End time, ISO 8601 or date string (default: now) |
| `--start` | Start time in ISO 8601 UTC format, e.g. `2026-01-15T08:00:00Z` (default: 24 hours ago) |
| `--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 |
|------|---------------|
| `Average` | `AggregateFunction_Average` |
| `Minimum` | `AggregateFunction_Minimum` |
| `Maximum` | `AggregateFunction_Maximum` |
| `Count` | `AggregateFunction_Count` |
| `Start` | `AggregateFunction_Start` |
| `End` | `AggregateFunction_End` |
| Name | Aliases | OPC UA Node ID |
|------|---------|---------------|
| `Average` | `avg` | `AggregateFunction_Average` |
| `Minimum` | `min` | `AggregateFunction_Minimum` |
| `Maximum` | `max` | `AggregateFunction_Maximum` |
| `Count` | | `AggregateFunction_Count` |
| `Start` | `first` | `AggregateFunction_Start` |
| `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,
out _);
ct).ConfigureAwait(false);
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,
out _);
ct).ConfigureAwait(false);
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
/// resolved via <see cref="ClientStoragePaths"/> so the one-shot legacy-folder migration
/// runs before the path is returned.
/// Path to the certificate store. Defaults to <see cref="string.Empty"/>; the
/// consuming application configuration factory resolves the canonical path via
/// <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(
conditionObjId,
acknowledgeMethodId,
[eventId, new LocalizedText(comment)],
ct);
// CallMethodAsync throws ServiceResultException on a bad call result;
// surface that as the returned StatusCode so callers using the documented
// `Task<StatusCode>` contract (e.g. `if (StatusCode.IsBad(result))`) see
// the failure instead of an uncaught exception they did not anticipate.
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);
var health = driver.GetHealth();
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync(["__probe"], ct);
var health = driver.GetHealth();
await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}");
await console.Output.WriteLineAsync($"Series: {Series}");
await console.Output.WriteLineAsync($"Health: {health.State}");
if (health.LastError is { } err)
await console.Output.WriteLineAsync($"Last error: {err}");
await console.Output.WriteLineAsync();
await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await console.Output.WriteLineAsync($"CNC: {CncHost}:{CncPort}");
await console.Output.WriteLineAsync($"Series: {Series}");
await console.Output.WriteLineAsync($"Health: {health.State}");
if (health.LastError is { } err)
await console.Output.WriteLineAsync($"Last error: {err}");
await console.Output.WriteLineAsync();
await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
}
@@ -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 console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await driver.InitializeAsync("{}", ct);
var snapshot = await driver.ReadAsync([tagName], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.Format(Address, snapshot[0]));
}
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}] " +
$"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
$"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
console.Output.WriteLine(line);
// Swallow + log write failures so a transient stdout error (closed pipe, IO
// exception on a redirected stream) cannot tear down the poll-engine
// background loop. Without this guard the unhandled exception would fault
// the long-running subscribe.
try
{
var line = $"[{DateTime.UtcNow:HH:mm:ss.fff}] " +
$"{e.FullReference} = {SnapshotFormatter.FormatValue(e.Snapshot.Value)} " +
$"({SnapshotFormatter.FormatStatus(e.Snapshot.StatusCode)})";
lock (writeLock)
{
console.Output.WriteLine(line);
}
}
catch (Exception ex)
{
Serilog.Log.Logger.Warning(ex,
"SubscribeCommand: console write failed for {Tag}; continuing poll loop.",
e.FullReference);
}
};
handle = await driver.SubscribeAsync([tagName], TimeSpan.FromMilliseconds(IntervalMs), ct);
await console.Output.WriteLineAsync(
$"Subscribed to {Address} @ {IntervalMs}ms. Ctrl+C to stop.");
// Driver.FOCAS.Cli-002: hold the lock around the banner write so the first
// 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 console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
}
finally
{
await driver.ShutdownAsync(CancellationToken.None);
}
await driver.InitializeAsync("{}", ct);
var results = await driver.WriteAsync([new WriteRequest(tagName, parsed)], ct);
await console.Output.WriteLineAsync(SnapshotFormatter.FormatWrite(Address, results[0]));
}
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),
FocasDataType.Byte => sbyte.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int16 => short.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Int32 => int.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float32 => float.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.Float64 => double.Parse(raw, CultureInfo.InvariantCulture),
FocasDataType.String => raw,
_ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."),
};
if (type == FocasDataType.Bit) return ParseBool(raw);
if (type == FocasDataType.String) return raw;
try
{
return type switch
{
FocasDataType.Byte => (object)sbyte.Parse(raw, CultureInfo.InvariantCulture),
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);
ThrowIfFailed(reply.Success, reply.Error, "ReadRaw");
var reply = await InvokeAndClassifyAsync<ReadRawRequest, ReadRawReply>(
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);
ThrowIfFailed(reply.Success, reply.Error, "ReadProcessed");
var reply = await InvokeAndClassifyAsync<ReadProcessedRequest, ReadProcessedReply>(
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);
ThrowIfFailed(reply.Success, reply.Error, "ReadAtTime");
var reply = await InvokeAndClassifyAsync<ReadAtTimeRequest, ReadAtTimeReply>(
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);
ThrowIfFailed(reply.Success, reply.Error, "ReadEvents");
var reply = await InvokeAndClassifyAsync<ReadEventsRequest, ReadEventsReply>(
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>(
MessageKind.WriteAlarmEventsRequest, MessageKind.WriteAlarmEventsReply, req, cancellationToken).ConfigureAwait(false);
var reply = await InvokeAsync<WriteAlarmEventsRequest, WriteAlarmEventsReply>(
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>(
MessageKind requestKind, MessageKind expectedReplyKind, TRequest request, CancellationToken ct)
/// <summary>
/// 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.
_totalSuccesses--;
_totalFailures++;
_consecutiveFailures++;
_lastFailureUtc = DateTime.UtcNow;
_lastError = message;
_totalQueries++;
if (success)
{
_totalSuccesses++;
_consecutiveFailures = 0;
_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
/// non-blocking (just resets transport state + disposes streams), so the
/// GetAwaiter()/GetResult() bridge is safe.
/// <see cref="IHistorianDataSource"/>. The underlying channel's async cleanup runs
/// <see cref="System.IO.Pipes.NamedPipeClientStream"/> teardown, which can block briefly
/// 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? ReconnectInitialBackoff = null,
TimeSpan? ReconnectMaxBackoff = null)
TimeSpan? CallTimeout = 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");
settings.CertificateStorePath.ShouldContain("pki");
// CertificateStorePath defaults to empty so constructing settings does not
// 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);
}
}