fix(driver-ablegacy-cli): resolve Low code-review findings (Driver.AbLegacy.Cli-002,003,004,005,006,007)
- Driver.AbLegacy.Cli-002: WriteCommand.Value description lists the full true/false, 1/0, on/off, yes/no alias set. - Driver.AbLegacy.Cli-003: SubscribeCommand serialises every WriteLine via a per-execution consoleGate lock so the poll-thread OnDataChange handler can't interleave with the banner. - Driver.AbLegacy.Cli-004: dropped 'await using var driver' in favour of a plain 'var driver' + explicit await ShutdownAsync in finally; the driver is no longer shut down twice. - Driver.AbLegacy.Cli-005: SubscribeCommand.IntervalMs description carries the PollGroupEngine 250ms-floor caveat; docs/Driver.AbLegacy.Cli.md spells out the same. - Driver.AbLegacy.Cli-006: ProbeCommand --type now carries the short alias 't' to match the other commands. - Driver.AbLegacy.Cli-007: BuildOptionsTests cover the probe-disabled, device-shape, tag-passthrough, timeout-propagation, and empty-tag-list paths. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -7,7 +7,7 @@
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 6 |
|
||||
| Open findings | 0 |
|
||||
|
||||
## Checklist coverage
|
||||
|
||||
@@ -68,7 +68,7 @@ type (mirroring the existing `Bit` and unsupported-type branches). Either catch
|
||||
| Severity | Low |
|
||||
| Category | Correctness & logic bugs |
|
||||
| Location | `Commands/WriteCommand.cs:27-29`, `Program.cs:6-9` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The `--value` option help text states "booleans accept
|
||||
true/false/1/0", but `ParseBool` (`WriteCommand.cs:74-80`) and the error message
|
||||
@@ -82,7 +82,10 @@ with both the code and the design doc.
|
||||
matching the wording used elsewhere (e.g. "booleans accept
|
||||
true/false, 1/0, on/off, yes/no").
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — updated `WriteCommand.Value` description to
|
||||
list the full alias set: "booleans accept true/false, 1/0, on/off, yes/no".
|
||||
Regression test `CommandMetadataTests.WriteCommand_value_help_lists_full_boolean_alias_set`
|
||||
asserts the description contains every alias group.
|
||||
|
||||
### Driver.AbLegacy.Cli-003
|
||||
|
||||
@@ -91,7 +94,7 @@ true/false, 1/0, on/off, yes/no").
|
||||
| Severity | Low |
|
||||
| Category | Concurrency & thread safety |
|
||||
| Location | `Commands/SubscribeCommand.cs:47-53` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The `OnDataChange` handler calls `console.Output.WriteLine(line)`
|
||||
(the synchronous overload) directly from the `PollGroupEngine` poll thread. The
|
||||
@@ -109,7 +112,10 @@ change events through a `Channel<string>` drained by a single consumer task, or
|
||||
guard the `WriteLine` with a lock. At minimum, document that the interleaving is
|
||||
accepted because output is human-facing and line-buffered.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — `SubscribeCommand` now serialises every
|
||||
console write through a shared `consoleGate` lock: the poll-thread
|
||||
`OnDataChange` callback and the command-thread "Subscribed to ..." line both take
|
||||
the lock before calling `WriteLine`. Comment in the source documents the intent.
|
||||
|
||||
### Driver.AbLegacy.Cli-004
|
||||
|
||||
@@ -118,7 +124,7 @@ accepted because output is human-facing and line-buffered.
|
||||
| Severity | Low |
|
||||
| Category | Error handling & resilience |
|
||||
| Location | `Commands/ProbeCommand.cs:37-56`, `Commands/ReadCommand.cs:39-50`, `Commands/WriteCommand.cs:48-59`, `Commands/SubscribeCommand.cs:41-76` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** Every command does `await using var driver = new AbLegacyDriver(...)`
|
||||
*and* an explicit `await driver.ShutdownAsync(...)` in the `finally`. `AbLegacyDriver`
|
||||
@@ -135,7 +141,12 @@ cleanup on every exit path including exceptions.
|
||||
since the commands deliberately pass `CancellationToken.None` to shutdown so teardown
|
||||
is not cut short by a cancelled `ct`.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — replaced `await using var driver` with a
|
||||
plain `var driver` in all four commands (`ProbeCommand`, `ReadCommand`,
|
||||
`WriteCommand`, `SubscribeCommand`), keeping the explicit
|
||||
`finally { await driver.ShutdownAsync(CancellationToken.None) }` as the single
|
||||
teardown path. Comment in each command documents the intent so readers do not
|
||||
have to verify idempotency.
|
||||
|
||||
### Driver.AbLegacy.Cli-005
|
||||
|
||||
@@ -144,7 +155,7 @@ is not cut short by a cancelled `ct`.
|
||||
| Severity | Low |
|
||||
| Category | Design-document adherence |
|
||||
| Location | `Commands/SubscribeCommand.cs:23-25`, `docs/Driver.AbLegacy.Cli.md:94-96` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The subscribe command interval option is `--interval-ms`
|
||||
(default 1000). `docs/Driver.AbLegacy.Cli.md` shows the subscribe example as
|
||||
@@ -160,7 +171,13 @@ but the documented contract drifts between the two CLIs.
|
||||
`--interval-ms` description for parity with the AbCip CLI, and mention the
|
||||
`--interval-ms` long form + 1000 ms default in `docs/Driver.AbLegacy.Cli.md`.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — extended the `SubscribeCommand.IntervalMs`
|
||||
help text to match AbCip ("Publishing interval in milliseconds (default 1000).
|
||||
PollGroupEngine floors sub-250ms values.") and added a paragraph under the
|
||||
`subscribe` section in `docs/Driver.AbLegacy.Cli.md` naming the `-i` /
|
||||
`--interval-ms` long form, the 1000 ms default, and the 250 ms floor. Regression
|
||||
test `CommandMetadataTests.SubscribeCommand_interval_ms_help_notes_PollGroupEngine_floor`
|
||||
asserts the description mentions "250".
|
||||
|
||||
### Driver.AbLegacy.Cli-006
|
||||
|
||||
@@ -169,7 +186,7 @@ but the documented contract drifts between the two CLIs.
|
||||
| Severity | Low |
|
||||
| Category | Code organization & conventions |
|
||||
| Location | `Commands/ProbeCommand.cs:20-22` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ProbeCommand` declares its `--type` option with no short alias,
|
||||
while `ReadCommand`, `WriteCommand`, and `SubscribeCommand` all declare `--type`
|
||||
@@ -182,7 +199,13 @@ it silently rejected on `probe`.
|
||||
for consistency with the other three commands. (The AbCip CLI `ProbeCommand` has
|
||||
the same omission, so a cross-CLI sweep is worthwhile.)
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — added the `'t'` short alias to
|
||||
`ProbeCommand.DataType`. Regression test
|
||||
`CommandMetadataTests.ProbeCommand_type_has_short_alias_t` (plus the parity test
|
||||
`Other_commands_keep_type_short_alias_t` for read/write/subscribe) asserts the
|
||||
short alias is present on every command. The same omission still exists in the
|
||||
AbCip CLI's `ProbeCommand` — flagged as a sibling sweep but out of scope for
|
||||
this module.
|
||||
|
||||
### Driver.AbLegacy.Cli-007
|
||||
|
||||
@@ -191,7 +214,7 @@ the same omission, so a cross-CLI sweep is worthwhile.)
|
||||
| Severity | Low |
|
||||
| Category | Testing coverage |
|
||||
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Cli.Tests/WriteCommandParseValueTests.cs` |
|
||||
| Status | Open |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** The only test file in the CLI test project covers
|
||||
`WriteCommand.ParseValue` and `ReadCommand.SynthesiseTagName`. Two behaviours that
|
||||
@@ -210,4 +233,11 @@ Driver.AbLegacy.Cli-001. `BuildOptions` is reachable via `InternalsVisibleTo`
|
||||
tag passthrough) and an overflow-input test for `ParseValue` so the fix for
|
||||
Driver.AbLegacy.Cli-001 is locked in by a regression test.
|
||||
|
||||
**Resolution:** _(open)_
|
||||
**Resolution:** Resolved 2026-05-23 — added `BuildOptionsTests` (five tests:
|
||||
probe disabled, device shape from Gateway+PlcType, tag passthrough, timeout
|
||||
propagation, empty tag list) covering `AbLegacyCommandBase.BuildOptions` via a
|
||||
nested `TestCommand` subclass annotated with `[Command]` to satisfy the CliFx
|
||||
analyzer. The overflow path for `ParseValue` is already covered by
|
||||
`WriteCommandParseValueTests.ParseValue_out_of_range_throws_CommandException`
|
||||
(theory with `short.Parse` + `AnalogInt` overflow inputs), added when finding
|
||||
Driver.AbLegacy.Cli-001 was resolved.
|
||||
|
||||
Reference in New Issue
Block a user