review(Driver.AbCip.Cli): fix stale CLI-count + misleading --type help
Re-review at 7286d320. -009: 'four'->'six' driver-CLI count in Program.cs. -010: ReadCommand
--type help no longer lists Structure (rejected at runtime) + pinning test.
This commit is contained in:
Joseph Doherty
@@ -4,8 +4,8 @@
|
||||
|---|---|
|
||||
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli` |
|
||||
| Reviewer | Claude Code |
|
||||
| Review date | 2026-05-22 |
|
||||
| Commit reviewed | `76d35d1` |
|
||||
| Review date | 2026-06-19 |
|
||||
| Commit reviewed | `c95a8c6b` |
|
||||
| Status | Reviewed |
|
||||
| Open findings | 0 |
|
||||
|
||||
@@ -266,3 +266,69 @@ drop the explicit count and link `docs/DriverClis.md` as the authoritative roste
|
||||
**Resolution:** Resolved 2026-05-23 — rewrote the lead paragraph to "Second of six
|
||||
driver test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS)"
|
||||
and added a link to `docs/DriverClis.md` as the authoritative roster.
|
||||
|
||||
## Re-review 2026-06-19 (commit c95a8c6b)
|
||||
|
||||
All eight findings from the 2026-05-22 review were confirmed resolved at this commit.
|
||||
The four commits that landed between the prior review and HEAD (two fix commits resolving -001 through -008, one XML-doc backfill, one central-package-management migration) were inspected. Two new low-severity findings were recorded and immediately fixed.
|
||||
|
||||
#### Re-review checklist coverage
|
||||
|
||||
| # | Category | Result |
|
||||
|---|---|---|
|
||||
| 1 | Correctness & logic bugs | No new issues found |
|
||||
| 2 | OtOpcUa conventions | No issues found |
|
||||
| 3 | Concurrency & thread safety | No new issues found (Driver.AbCip.Cli-003 confirmed resolved) |
|
||||
| 4 | Error handling & resilience | No new issues found |
|
||||
| 5 | Security | No issues found |
|
||||
| 6 | Performance & resource management | No new issues found |
|
||||
| 7 | Design-document adherence | No new issues found |
|
||||
| 8 | Code organization & conventions | No issues found |
|
||||
| 9 | Testing coverage | No new gaps found |
|
||||
| 10 | Documentation & comments | Driver.AbCip.Cli-009, Driver.AbCip.Cli-010 |
|
||||
|
||||
### Driver.AbCip.Cli-009
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Program.cs:9` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `Program.cs` `SetDescription` still says "Second of **four** driver CLIs"
|
||||
after finding Driver.AbCip.Cli-008 corrected the matching sentence in `docs/Driver.AbCip.Cli.md`.
|
||||
The FOCAS and TwinCAT CLIs ship alongside the other four, making six total; the stale count
|
||||
in the process banner is visible in `--help` output.
|
||||
|
||||
**Recommendation:** Change "four" to "six" in `Program.cs:9` to match the corrected
|
||||
`docs/Driver.AbCip.Cli.md` wording.
|
||||
|
||||
**Resolution:** Resolved 2026-06-19 (SHA blank) — changed "Second of four driver CLIs" to
|
||||
"Second of six driver CLIs" in `Program.cs:9`.
|
||||
|
||||
### Driver.AbCip.Cli-010
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Severity | Low |
|
||||
| Category | Documentation & comments |
|
||||
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs:26` |
|
||||
| Status | Resolved |
|
||||
|
||||
**Description:** `ReadCommand`'s `--type` `[CommandOption]` description lists
|
||||
`Structure` as a valid type value (`"... / Dt / Structure (default DInt)."`).
|
||||
Finding Driver.AbCip.Cli-002 added `RejectStructure(DataType)` at the top of
|
||||
`ReadCommand.ExecuteAsync`, so passing `--type Structure` is refused at runtime
|
||||
with a `CommandException`. The help text still advertises `Structure` as an accepted
|
||||
option, misleading operators who will get a rejection immediately after selecting it.
|
||||
|
||||
**Recommendation:** Remove `Structure` from the `--type` description and replace it
|
||||
with a note indicating UDTs are unsupported in the CLI, mirroring the wording used in
|
||||
the `RejectStructure` exception message.
|
||||
|
||||
**Resolution:** Resolved 2026-06-19 (SHA blank) — replaced `"String / Dt / Structure (default DInt)."` with
|
||||
`"String / Dt (default DInt). UDT / composite types are not supported here — use a full driver config."`
|
||||
in `ReadCommand`'s `--type` description; added a pinning test
|
||||
`HelpTextConsistencyTests.ReadCommand_type_option_description_does_not_list_Structure`
|
||||
(new file `tests/.../HelpTextConsistencyTests.cs`) to guard against reintroduction.
|
||||
|
||||
@@ -23,7 +23,7 @@ public sealed class ReadCommand : AbCipCommandBase
|
||||
/// <summary>Gets the data type to read as.</summary>
|
||||
[CommandOption("type", Description =
|
||||
"Bool / SInt / Int / DInt / LInt / USInt / UInt / UDInt / ULInt / Real / LReal / " +
|
||||
"String / Dt / Structure (default DInt).")]
|
||||
"String / Dt (default DInt). UDT / composite types are not supported here — use a full driver config.")]
|
||||
public AbCipDataType DataType { get; init; } = AbCipDataType.DInt;
|
||||
|
||||
/// <summary>Executes the read operation.</summary>
|
||||
|
||||
@@ -6,6 +6,6 @@ return await new CliApplicationBuilder()
|
||||
.SetDescription(
|
||||
"OtOpcUa AB CIP test-client — ad-hoc probe + Logix symbolic reads/writes + polled " +
|
||||
"subscriptions against ControlLogix / CompactLogix / Micro800 / GuardLogix families " +
|
||||
"via libplctag. Second of four driver CLIs; mirrors otopcua-modbus-cli's shape.")
|
||||
"via libplctag. Second of six driver CLIs; mirrors otopcua-modbus-cli's shape.")
|
||||
.Build()
|
||||
.RunAsync(args);
|
||||
|
||||
+37
@@ -0,0 +1,37 @@
|
||||
using System.Reflection;
|
||||
using CliFx.Attributes;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Commands;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Verifies that the CLI's help text is consistent with its runtime behaviour — in
|
||||
/// particular that rejected types do not appear as valid options in the option description,
|
||||
/// and that counts stated in CLI metadata are accurate.
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class HelpTextConsistencyTests
|
||||
{
|
||||
/// <summary>
|
||||
/// Driver.AbCip.Cli-010 — ReadCommand's <c>--type</c> option description must NOT
|
||||
/// list <c>Structure</c>, because <c>RejectStructure</c> will refuse it at runtime.
|
||||
/// Listing a rejected value in the help text misleads operators.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void ReadCommand_type_option_description_does_not_list_Structure()
|
||||
{
|
||||
var prop = typeof(ReadCommand).GetProperty(
|
||||
nameof(ReadCommand.DataType),
|
||||
BindingFlags.Public | BindingFlags.Instance)
|
||||
?? throw new InvalidOperationException("DataType property not found on ReadCommand");
|
||||
|
||||
var attr = prop.GetCustomAttribute<CommandOptionAttribute>()
|
||||
?? throw new InvalidOperationException("CommandOptionAttribute not found on ReadCommand.DataType");
|
||||
|
||||
// RejectStructure is called in ReadCommand.ExecuteAsync; listing Structure in the
|
||||
// help text as a valid --type value misleads operators (Driver.AbCip.Cli-010).
|
||||
(attr.Description ?? string.Empty).ShouldNotContain("Structure");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user