dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-twincat-cli): resolve Low code-review findings (Driver.TwinCAT.Cli-001,002,003,004,005,006,007)

Browse Source 
- Driver.TwinCAT.Cli-001: TwinCATCommandBase.Validate rejects
  non-positive TimeoutMs / IntervalMs and AmsPort outside 1..65535;
  ExecuteAsync calls it first.
- Driver.TwinCAT.Cli-002: SubscribeCommand serialises every WriteLine
  through a writeLock to remove the notification-callback vs banner
  interleave risk.
- Driver.TwinCAT.Cli-003: SubscribeCommand.DescribeMechanism derives
  the banner label from the returned ISubscriptionHandle.DiagnosticId
  so it can't disagree with what the driver actually did.
- Driver.TwinCAT.Cli-004: introduced TwinCATTagCommandBase carrying
  --poll-only + BuildOptions; BrowseCommand stays on the slimmer
  TwinCATCommandBase so --poll-only no longer surfaces in browse --help.
- Driver.TwinCAT.Cli-005: ProbeCommand --type now carries the 't' short
  alias to match the other commands.
- Driver.TwinCAT.Cli-006: 35 new tests covering Gateway / AmsAddress
  parse / BuildOptions / PollOnly / browse-helpers / probe-alias /
  mechanism derivation.
- Driver.TwinCAT.Cli-007: replaced the empty-init <inheritdoc/> with an
  explicit summary warning future maintainers about the no-op init.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-23 08:34:57 -04:00
parent 67ef6c4ebc
commit f2ee027145
11 changed files with 642 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

81
code-reviews/Driver.TwinCAT.Cli/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 7 |
| Open findings | 0 |
## Checklist coverage
@@ -40,7 +40,7 @@ a category produced nothing rather than leaving it blank.
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `TwinCATCommandBase.cs:23-24`, `Commands/SubscribeCommand.cs:23-24`, `Commands/BrowseCommand.cs:21-24` |
| Status | Open |
| Status | Resolved |
**Description:** Numeric command options are accepted without range validation. `--timeout-ms`
feeds `Timeout => TimeSpan.FromMilliseconds(TimeoutMs)`; passing `--timeout-ms 0` or a negative
@@ -56,7 +56,16 @@ failure mode should be a readable up-front rejection.
shared helper on `TwinCATCommandBase`) and throw `CliFx.Exceptions.CommandException` with a
clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outside `1..65535`.
**Resolution:** _(open)_
**Resolution:** Added a virtual `Validate()` helper on `TwinCATCommandBase` that rejects
`TimeoutMs <= 0` and `AmsPort` outside `1..65535` with a clean
`CliFx.Exceptions.CommandException` carrying the offending value. `SubscribeCommand` overrides
`Validate()` to add the `IntervalMs > 0` check. Each `ExecuteAsync` calls `Validate()` first
so the CLI surfaces "bad argument" up-front instead of letting the driver fail with an opaque
transport error. Covered by `TwinCATCommandBaseTests.Validate_rejects_zero_timeout`,
`Validate_rejects_negative_timeout`, `Validate_rejects_out_of_range_ams_port` (theory, 4
cases), `Validate_accepts_in_range_ams_port` (theory, 4 cases),
`SubscribeCommand_validate_rejects_zero_interval`,
`SubscribeCommand_validate_rejects_negative_interval`.
### Driver.TwinCAT.Cli-002
@@ -65,7 +74,7 @@ clear message when `TimeoutMs <= 0`, `IntervalMs <= 0`, or `AmsPort` falls outsi
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `Commands/SubscribeCommand.cs:46-58` |
| Status | Open |
| Status | Resolved |
**Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)` synchronously.
In native ADS-notification mode the event is raised from the `Beckhoff.TwinCAT.Ads`
@@ -83,7 +92,12 @@ serialised on one poll loop; the TwinCAT native path has no such serialisation.
`TextWriter.Synchronized`. At minimum, gate it so the banner is written before the
subscription is registered (it already is) and lock the per-event writes against each other.
**Resolution:** _(open)_
**Resolution:** Introduced a per-execution `writeLock` object inside
`SubscribeCommand.ExecuteAsync`. Both the `OnDataChange` handler's `WriteLine` and the
post-subscription "Subscribed to ..." banner take the lock, so notification-callback writes
cannot interleave with the main-thread banner or with each other. Lock is local to the
command so parallel process instances do not contend with one another (each owns its own
console).
### Driver.TwinCAT.Cli-003
@@ -92,7 +106,7 @@ subscription is registered (it already is) and lock the per-event writes against
| Severity | Low |
| Category | Error handling & resilience |
| Location | `Commands/SubscribeCommand.cs:56-58` |
| Status | Open |
| Status | Resolved |
**Description:** The subscribe banner reports the mechanism purely from the `--poll-only` flag
(`var mode = PollOnly ? "polling" : "ADS notification"`). The doc (`docs/Driver.TwinCAT.Cli.md`)
@@ -108,7 +122,15 @@ returned `ISubscriptionHandle.DiagnosticId`, which is `twincat-native-sub-*` for
path vs the `PollGroupEngine` handle for poll mode) or soften the wording to "(requested:
ADS notification)" so it does not over-claim.
**Resolution:** _(open)_
**Resolution:** Added internal static
`SubscribeCommand.DescribeMechanism(ISubscriptionHandle)` that maps
`DiagnosticId.StartsWith("twincat-native-sub-", Ordinal)` to `"ADS notification"` and
anything else to `"polling"`. The banner now reads from the handle the driver actually
returned, so the line cannot disagree with what the driver did even if a future fallback
lands the subscription somewhere unexpected. Covered by
`SubscribeCommandMechanismTests.DescribeMechanism_returns_ADS_notification_for_native_handle`
(theory, 3 cases) and `DescribeMechanism_returns_polling_for_anything_else` (theory, 4 cases
including an ordinal case-sensitivity guard).
### Driver.TwinCAT.Cli-004
@@ -117,7 +139,7 @@ ADS notification)" so it does not over-claim.
| Severity | Low |
| Category | Design-document adherence |
| Location | `TwinCATCommandBase.cs:26-29`, `Commands/BrowseCommand.cs` |
| Status | Open |
| Status | Resolved |
**Description:** `--poll-only` is declared on `TwinCATCommandBase`, so it is inherited by
`browse`. `BrowseCommand` only ever calls `DiscoverAsync` — it never subscribes — so
@@ -131,7 +153,15 @@ disagree.
flag) onto an intermediate base shared by only `probe`/`read`/`subscribe`, or override/hide it
for `browse`. Alternatively document explicitly that the flag is a no-op for `browse`.
**Resolution:** _(open)_
**Resolution:** Introduced an intermediate `TwinCATTagCommandBase : TwinCATCommandBase` that
hosts the `--poll-only` flag and the `BuildOptions(...)` helper. `ProbeCommand`,
`ReadCommand`, `WriteCommand`, and `SubscribeCommand` inherit from this intermediate (they all
build a tag-list `TwinCATDriverOptions`). `BrowseCommand` keeps inheriting from
`TwinCATCommandBase` directly, so `--poll-only` no longer surfaces in `browse --help`. Browse
sets `UseNativeNotifications = true` on its inline options (irrelevant either way for the
discover-only path, but matches production wiring). Covered by
`TwinCATCommandBaseTests.BrowseCommand_does_not_expose_poll_only_flag` and
`ProbeCommand_still_exposes_poll_only_flag` (both reflect over the public property surface).
### Driver.TwinCAT.Cli-005
@@ -140,7 +170,7 @@ for `browse`. Alternatively document explicitly that the flag is a no-op for `br
| Severity | Low |
| Category | Code organization & conventions |
| Location | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` |
| Status | Open |
| Status | Resolved |
**Description:** The `--type` option is declared with the short alias `-t` on `read`, `write`,
and `subscribe`, but `ProbeCommand` declares `[CommandOption("type", ...)]` with no short
@@ -151,7 +181,10 @@ take the same `TwinCATDataType` option.
**Recommendation:** Add the `'t'` short alias to `ProbeCommand`'s `--type` option to match the
other three commands.
**Resolution:** _(open)_
**Resolution:** Added the `'t'` short alias on `ProbeCommand.DataType`'s `[CommandOption]`,
matching read/write/subscribe so muscle memory carries between the four verbs. Covered by
`TwinCATCommandBaseTests.ProbeCommand_type_option_carries_short_alias_t` which asserts the
`CommandOptionAttribute.ShortName` is `'t'`.
### Driver.TwinCAT.Cli-006
@@ -160,7 +193,7 @@ other three commands.
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The only test file covers `WriteCommand.ParseValue` and
`ReadCommand.SynthesiseTagName`. Other deterministic, router-independent logic is untested:
@@ -177,7 +210,20 @@ module's scope but worth flagging to whoever owns the test tree.
`BuildOptions` field wiring, and for the `CollectingAddressSpaceBuilder` prefix/max filtering
and access-classification logic.
**Resolution:** _(open)_
**Resolution:** Added two new test classes. `TwinCATCommandBaseTests` covers the Gateway
canonical form, round-trip through `TwinCATAmsAddress.TryParse`, DriverInstanceId
composition, Timeout projection, BuildOptions field wiring (devices, tags, timeout, probe
disabled, controller-browse disabled, UseNativeNotifications default true), and the PollOnly
toggle flipping UseNativeNotifications. `BrowseCommandFilterTests` covers
`CollectingAddressSpaceBuilder` (records variables in call order, treats `Folder` as a
same-builder pass-through), `BrowseCommand.FilterByPrefix` (empty/null prefix passes
everything, case-sensitive ordinal match), `BrowseCommand.PrintLimit` (max <= 0 = unbounded,
caps when matched > max, no padding when matched < max), and `BrowseCommand.AccessTag`
(ViewOnly -> RO, every other classification -> RW, theory over all 6 non-ViewOnly values).
`BrowseCommand.CollectingAddressSpaceBuilder` made `internal` (was `private`) so the test
project can construct it directly via the existing `InternalsVisibleTo` hook. Total tests
for this assembly went from 27 to 69. The stale empty sibling test directory mention is
left out of scope as noted.
### Driver.TwinCAT.Cli-007
@@ -186,7 +232,7 @@ and access-classification logic.
| Severity | Low |
| Category | Documentation & comments |
| Location | `TwinCATCommandBase.cs:31-36` |
| Status | Open |
| Status | Resolved |
**Description:** The `Timeout` override has an empty `init` accessor with the comment
`/* driven by TimeoutMs */`. Because the base `DriverCommandBase.Timeout` is declared
@@ -199,4 +245,9 @@ gives no hint of the deliberate no-op. This is a maintainability/clarity nit, no
computed projection of `--timeout-ms` and the `init` accessor is intentionally a no-op, so the
design intent survives refactoring.
**Resolution:** _(open)_
**Resolution:** Replaced the `<inheritdoc/>` on `TwinCATCommandBase.Timeout` with an explicit
`<summary>` documenting that `Timeout` is projected from `TimeoutMs`, that the `init`
accessor required by the abstract base property is intentionally a no-op, and that adding a
backing field would cause the two to drift on every refactor. The inner-block comment was
tightened to point at the XML summary so the design intent survives whichever doc surface a
future maintainer reads first.