lmxopcua/code-reviews/Driver.TwinCAT.Cli/findings.md

# Code Review — Driver.TwinCAT.Cli

| Field | Value |
|---|---|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 0 |

## Checklist coverage

A comprehensive review completes every category, recording "No issues found" where
a category produced nothing rather than leaving it blank.

| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.TwinCAT.Cli-001 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.TwinCAT.Cli-002 |
| 4 | Error handling & resilience | Driver.TwinCAT.Cli-003 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | Driver.TwinCAT.Cli-004 |
| 8 | Code organization & conventions | Driver.TwinCAT.Cli-005 |
| 9 | Testing coverage | Driver.TwinCAT.Cli-006 |
| 10 | Documentation & comments | Driver.TwinCAT.Cli-007 |

## Findings

<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
     never reused. Findings are never deleted — close them by changing Status and
     completing Resolution. -->

### Driver.TwinCAT.Cli-001

| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` |
| Status | Resolved |

**Description:** Numeric command options are accepted without range validation. `--timeout-ms`
feeds `Timeout => TimeSpan.FromMilliseconds(TimeoutMs)`; passing `--timeout-ms 0` or a negative
value yields `TimeSpan.Zero`/a negative `TimeSpan`, which is then handed to the driver's
`TwinCATDriverOptions.Timeout` and on to `ITwinCATClient.ConnectAsync`, producing an immediate
failure or undefined behaviour rather than a clear "bad argument" message. The same applies to
`subscribe --interval-ms` (negative -> `TimeSpan.FromMilliseconds(negative)` passed to
`SubscribeAsync`) and `--ams-port` (`AmsPort` accepts negative / out-of-range port numbers,
which only surface later as an opaque transport error). For a commissioning/diagnostic tool the
failure mode should be a readable up-front rejection.

**Recommendation:** Validate the numeric options at the top of each `ExecuteAsync` (or via a
shared helper on `TwinCATCommandBase`) and throw `CliFx.Exceptions.CommandException` with a
clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outside `1..65535`.

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

### Driver.TwinCAT.Cli-002

| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `Commands/SubscribeCommand.cs:46-58` |
| Status | Resolved |

**Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)` synchronously.
In native ADS-notification mode the event is raised from the `Beckhoff.TwinCAT.Ads`
notification callback thread (see `TwinCATDriver.SubscribeAsync`, which invokes `OnDataChange`
from the ADS `AddNotificationAsync` callback). That write can interleave with the main thread's
`console.Output.WriteLineAsync(...)` "Subscribed to ..." banner and with subsequent change
events if the PLC pushes faster than a single write completes. A `TextWriter` is not guaranteed
thread-safe, so output lines can be garbled — undesirable for a tool whose stated purpose is
producing clean screen-recorded bug-report timelines. The same pattern exists in the other
driver CLIs (S7/Modbus), but those go through `PollGroupEngine`, whose change callbacks are
serialised on one poll loop; the TwinCAT native path has no such serialisation.

**Recommendation:** Serialise console writes from the change handler, e.g. wrap the
`WriteLine` body in a `lock` on a private object that the banner write also takes, or use
`TextWriter.Synchronized`. At minimum, gate it so the banner is written before the
subscription is registered (it already is) and lock the per-event writes against each other.

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

### Driver.TwinCAT.Cli-003

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `Commands/SubscribeCommand.cs:56-58` |
| Status | Resolved |

**Description:** The subscribe banner reports the mechanism purely from the `--poll-only` flag
(`var mode = PollOnly ? "polling" : "ADS notification"`). The doc (`docs/Driver.TwinCAT.Cli.md`)
states the banner "announces which mechanism is in play". The CLI always declares exactly one
tag, so a registration that produces zero notification handles is unlikely, but `TwinCATDriver.
SubscribeAsync` silently `continue`s past any reference not found in `_tagsByName`/`_devices`
and a poll-mode fallback inside the driver is also possible in principle. The banner therefore
asserts a mechanism it has not actually confirmed. It is informational only, so the impact is
limited to a misleading diagnostic line.

**Recommendation:** Either derive the banner text from observable subscription state (e.g. the
returned `ISubscriptionHandle.DiagnosticId`, which is `twincat-native-sub-*` for the native
path vs the `PollGroupEngine` handle for poll mode) or soften the wording to "(requested:
ADS notification)" so it does not over-claim.

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

### Driver.TwinCAT.Cli-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` |
| Status | Resolved |

**Description:** `--poll-only` is declared on `TwinCATCommandBase`, so it is inherited by
`browse`. `BrowseCommand` only ever calls `DiscoverAsync` — it never subscribes — so
`UseNativeNotifications = !PollOnly` has no observable effect on a browse run. The flag still
appears in `otopcua-twincat-cli browse --help`, implying it changes browse behaviour when it
does not. `docs/Driver.TwinCAT.Cli.md` documents `--poll-only` only under `subscribe` and lists
no per-command flags for `browse` beyond `--prefix`/`--max`, so the help text and the docs
disagree.

**Recommendation:** Move `--poll-only` (and arguably the notification-only relevance of the
flag) onto an intermediate base shared by only `probe`/`read`/`subscribe`, or override/hide it
for `browse`. Alternatively document explicitly that the flag is a no-op for `browse`.

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

### Driver.TwinCAT.Cli-005

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` |
| Status | Resolved |

**Description:** The `--type` option is declared with the short alias `-t` on `read`, `write`,
and `subscribe`, but `ProbeCommand` declares `[CommandOption("type", ...)]` with no short
alias. An operator who has internalised `-t` from the other three verbs will get a CliFx
"unknown option" error on `probe -t Bool`. The inconsistency is gratuitous — all four commands
take the same `TwinCATDataType` option.

**Recommendation:** Add the `'t'` short alias to `ProbeCommand`'s `--type` option to match the
other three commands.

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

### Driver.TwinCAT.Cli-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` |
| Status | Resolved |

**Description:** The only test file covers `WriteCommand.ParseValue` and
`ReadCommand.SynthesiseTagName`. Other deterministic, router-independent logic is untested:
`TwinCATCommandBase.Gateway` (the `ads://{netId}:{port}` string the driver's
`TwinCATAmsAddress.TryParse` consumes — a regression here breaks every command), `BuildOptions`
(tag wiring, `UseNativeNotifications` toggle, `Probe.Enabled = false`), and `BrowseCommand`'s
`CollectingAddressSpaceBuilder` with its `--prefix`/`--max` filtering and the `RO`/`RW` access
derivation. These are pure and can be unit-tested without an AMS router. `InternalsVisibleTo`
is already wired for the test assembly. Note also the stale empty sibling test directory
`tests/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests` (no project, no files) — out of this
module's scope but worth flagging to whoever owns the test tree.

**Recommendation:** Add unit tests for `Gateway`/`DriverInstanceId` string composition, for
`BuildOptions` field wiring, and for the `CollectingAddressSpaceBuilder` prefix/max filtering
and access-classification logic.

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

### Driver.TwinCAT.Cli-007

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `TwinCATCommandBase.cs:31-36` |
| Status | Resolved |

**Description:** The `Timeout` override has an empty `init` accessor with the comment
`/* driven by TimeoutMs */`. Because the base `DriverCommandBase.Timeout` is declared
`abstract { get; init; }`, the override must supply an `init`, but here it silently discards
any value. This is intentional, yet the empty body invites a future maintainer to "fix" it by
adding a backing field, which would then diverge from `TimeoutMs`. The XML `<inheritdoc/>`
gives no hint of the deliberate no-op. This is a maintainability/clarity nit, not a bug.

**Recommendation:** Replace `<inheritdoc/>` with a short summary stating that `Timeout` is a
computed projection of `--timeout-ms` and the `init` accessor is intentionally a no-op, so the
design intent survives refactoring.

**Resolution:** Replaced the `<inheritdoc/>` on `TwinCATCommandBase.Timeout` with an explicit
`<summary>` documenting that `Timeout` is projected from `TimeoutMs`, that the `init`
accessor required by the abstract base property is intentionally a no-op, and that adding a
backing field would cause the two to drift on every refactor. The inner-block comment was
tightened to point at the XML summary so the design intent survives whichever doc surface a
future maintainer reads first.