diff --git a/code-reviews/Driver.AbCip.Cli/findings.md b/code-reviews/Driver.AbCip.Cli/findings.md index 9ce9b4c..88fa810 100644 --- a/code-reviews/Driver.AbCip.Cli/findings.md +++ b/code-reviews/Driver.AbCip.Cli/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 6 | ## Checklist coverage @@ -40,7 +40,7 @@ a category produced nothing rather than leaving it blank. | Severity | Medium | | Category | Error handling & resilience | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs:70-85` | -| Status | Open | +| Status | Resolved | **Description:** `ParseValue` parses every numeric Logix type with the BCL `*.Parse` methods (`sbyte.Parse`, `short.Parse`, `int.Parse`, `float.Parse`, ...). These throw @@ -59,7 +59,7 @@ one-line error. CliFx only formats `CommandException` cleanly. rethrows as a `CommandException` with the raw value, the target `--type`, and the valid range — mirroring the `ParseBool` failure message. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — wrapped the `ParseValue` switch in `try/catch (FormatException or OverflowException)` that rethrows as `CommandException` with the raw value and type; updated the previously-passing `ParseValue_non_numeric_for_numeric_types_throws` test to assert `CommandException` and added two new tests covering overflow and actionable message content. ### Driver.AbCip.Cli-002 @@ -68,7 +68,7 @@ valid range — mirroring the `ParseBool` failure message. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs:21-23`; `Commands/ReadCommand.cs:24-25`; `Commands/SubscribeCommand.cs:20-22` | -| Status | Open | +| Status | Resolved | **Description:** `ProbeCommand`, `ReadCommand`, and `SubscribeCommand` expose `--type` as a free `AbCipDataType` enum option with no exclusion of @@ -87,7 +87,7 @@ are out of scope here", but the code does not enforce it. pattern `WriteCommand` uses, or factor a shared `RejectStructure(DataType)` guard into `AbCipCommandBase`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `RejectStructure(AbCipDataType)` static helper to `AbCipCommandBase` that throws `CommandException` for `Structure`; called at the top of `ExecuteAsync` in `ProbeCommand`, `ReadCommand`, and `SubscribeCommand`. ### Driver.AbCip.Cli-003 diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs index 7f8fe81..d683851 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs @@ -56,4 +56,19 @@ public abstract class AbCipCommandBase : DriverCommandBase /// multiple gateways in parallel can distinguish the logs. /// protected string DriverInstanceId => $"abcip-cli-{Gateway}"; + + /// + /// Guards against being passed to a command + /// that does not support UDT layouts. Call at the top of ExecuteAsync for any + /// command that accepts --type but cannot handle memberless Structure tags. + /// Throws a if + /// is . + /// + protected static void RejectStructure(AbCipDataType type) + { + if (type == AbCipDataType.Structure) + throw new CliFx.Exceptions.CommandException( + "Structure (UDT) reads are out of scope for this command — those need an explicit " + + "member layout, which belongs in a real driver config."); + } } diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs index bbe05e8..ca849cc 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs @@ -25,6 +25,7 @@ public sealed class ProbeCommand : AbCipCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + RejectStructure(DataType); var ct = console.RegisterCancellationHandler(); var probeTag = new AbCipTagDefinition( 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 3aaf546..deeb26f 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 @@ -27,6 +27,7 @@ public sealed class ReadCommand : AbCipCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + RejectStructure(DataType); var ct = console.RegisterCancellationHandler(); var tagName = SynthesiseTagName(TagPath, DataType); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs index 15a7a9c..beb8a2a 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs @@ -30,6 +30,7 @@ public sealed class SubscribeCommand : AbCipCommandBase public override async ValueTask ExecuteAsync(IConsole console) { ConfigureLogging(); + RejectStructure(DataType); var ct = console.RegisterCancellationHandler(); var tagName = ReadCommand.SynthesiseTagName(TagPath, DataType); diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs index 32ec025..8d4ed0f 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs @@ -66,23 +66,40 @@ public sealed class WriteCommand : AbCipCommandBase /// /// Parse the operator's --value string into the CLR type the driver expects /// for the declared . Invariant culture everywhere. + /// Bad input (non-numeric text, out-of-range value) is caught and rethrown as a + /// so CliFx renders a clean one-line + /// error rather than a full .NET stack trace. /// - internal static object ParseValue(string raw, AbCipDataType type) => type switch + internal static object ParseValue(string raw, AbCipDataType type) { - AbCipDataType.Bool => ParseBool(raw), - AbCipDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.DInt or AbCipDataType.Dt => int.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture), - AbCipDataType.String => raw, - _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), - }; + try + { + return type switch + { + AbCipDataType.Bool => ParseBool(raw), + AbCipDataType.SInt => sbyte.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.Int => short.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.DInt or AbCipDataType.Dt => int.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.LInt => long.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.USInt => byte.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.UInt => ushort.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.UDInt => uint.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.ULInt => ulong.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.Real => float.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.LReal => double.Parse(raw, CultureInfo.InvariantCulture), + AbCipDataType.String => raw, + _ => throw new CliFx.Exceptions.CommandException($"Unsupported DataType '{type}' for write."), + }; + } + catch (Exception ex) when (ex is FormatException or OverflowException) + { + throw new CliFx.Exceptions.CommandException( + $"Cannot parse '{raw}' as {type}. " + + $"Check the value is within the valid range for {type} and uses invariant-culture " + + $"decimal notation (e.g. '3.14', not '3,14').", + innerException: ex); + } + } private static bool ParseBool(string raw) => raw.Trim().ToLowerInvariant() switch { diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs index a6afb9b..e74c6c4 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs @@ -81,12 +81,33 @@ public sealed class WriteCommandParseValueTests } [Fact] - public void ParseValue_non_numeric_for_numeric_types_throws() + public void ParseValue_non_numeric_for_numeric_types_throws_CommandException() { - Should.Throw( + Should.Throw( () => WriteCommand.ParseValue("xyz", AbCipDataType.DInt)); } + [Fact] + public void ParseValue_out_of_range_throws_CommandException() + { + // sbyte max is 127; 999 overflows it. + Should.Throw( + () => WriteCommand.ParseValue("999", AbCipDataType.SInt)); + } + + [Theory] + [InlineData("12x", AbCipDataType.Int)] + [InlineData("3.14", AbCipDataType.DInt)] + [InlineData("99999999999", AbCipDataType.Int)] + public void ParseValue_bad_input_CommandException_message_is_actionable( + string raw, AbCipDataType type) + { + var ex = Should.Throw( + () => WriteCommand.ParseValue(raw, type)); + ex.Message.ShouldContain(raw); + ex.Message.ShouldContain(type.ToString()); + } + [Theory] [InlineData("Motor01_Speed", AbCipDataType.Real, "Motor01_Speed:Real")] [InlineData("Program:Main.Counter", AbCipDataType.DInt, "Program:Main.Counter:DInt")]