diff --git a/code-reviews/Driver.AbCip.Cli/findings.md b/code-reviews/Driver.AbCip.Cli/findings.md index b7cc018a..ac35256d 100644 --- a/code-reviews/Driver.AbCip.Cli/findings.md +++ b/code-reviews/Driver.AbCip.Cli/findings.md @@ -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. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs index 5bad9277..69f1421a 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ReadCommand.cs @@ -23,7 +23,7 @@ public sealed class ReadCommand : AbCipCommandBase /// Gets the data type to read as. [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; /// Executes the read operation. diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Program.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Program.cs index 6232f519..2f561e1d 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Program.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Program.cs @@ -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); diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/HelpTextConsistencyTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/HelpTextConsistencyTests.cs new file mode 100644 index 00000000..a9042169 --- /dev/null +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/HelpTextConsistencyTests.cs @@ -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; + +/// +/// 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. +/// +[Trait("Category", "Unit")] +public sealed class HelpTextConsistencyTests +{ + /// + /// Driver.AbCip.Cli-010 — ReadCommand's --type option description must NOT + /// list Structure, because RejectStructure will refuse it at runtime. + /// Listing a rejected value in the help text misleads operators. + /// + [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() + ?? 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"); + } +}