diff --git a/code-reviews/Client.CLI/findings.md b/code-reviews/Client.CLI/findings.md index 2278a6db..795c3699 100644 --- a/code-reviews/Client.CLI/findings.md +++ b/code-reviews/Client.CLI/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | @@ -269,3 +269,50 @@ The `FakeOpcUaClientService` already exposes `RaiseDataChanged`, so feeding good and asserting the summary text is straightforward. **Resolution:** Resolved 2026-05-23 — added `SubscribeCommandSummaryTests` (covering recursive collection via `FakeOpcUaClientService.AddDiscoveredVariable`, `--duration` auto-exit, summary bucketing for good/bad/never/never-went-bad, and the `--summary-file` write), `CommandRangeValidationTests`, `EventHandlerLifecycleTests`, `InputValidationErrorsTests`, and `LoggerLifecycleTests` to pin the other Low findings; `FakeOpcUaClientService` was extended with `AddDiscoveredVariable` / `RaiseDataChanged` helpers. + +## Re-review 2026-06-19 (commit 7286d320) + +Five new commands were added since the prior review (`76d35d1`): `ack`, `confirm`, `enable`, `disable`, `shelve`. All 10 checklist categories were re-examined against HEAD. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Client.CLI-011 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | Client.CLI-011 | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | Client.CLI-011 (new tests added as part of fix) | +| 10 | Documentation & comments | Client.CLI-012 | + +### Client.CLI-011 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs / Error handling & resilience | +| Location | `Commands/AcknowledgeCommand.cs`, `Commands/ConfirmCommand.cs`, `Commands/EnableCommand.cs`, `Commands/DisableCommand.cs`, `Commands/ShelveCommand.cs` | +| Status | Resolved | + +**Description:** The five alarm-operation commands (`ack`, `confirm`, `enable`, `disable`, `shelve`) pass `--node` directly to the shared `IOpcUaClientService` methods without first validating the node-id string through `NodeIdParser`. Inside the service, each method calls `NodeId.Parse(conditionNodeId)` before its own `try/catch (ServiceResultException)` block; a malformed value therefore throws a raw exception (not a `CommandException`) that propagates out of `ExecuteAsync` and surfaces as a stack trace, not a clean CLI error. This is exactly the class of issue fixed by Client.CLI-006 for `read`, `write`, `browse`, `subscribe`, and `historyread`, but the five new alarm-op commands were not updated to match. + +**Recommendation:** Call `NodeIdParser.ParseRequired(NodeId)` (discarding the result, since the service accepts a raw string) inside a `catch (FormatException or ArgumentException)` block that rethrows as `CommandException`, mirroring the pattern used by every other command in the module. + +**Resolution:** Resolved 2026-06-19 — all five alarm-op commands now call `NodeIdParser.ParseRequired(NodeId)` at the start of `ExecuteAsync`, catching `FormatException`/`ArgumentException` and rethrowing as `CommandException`. Pinned by `AlarmOpCommandNodeIdValidationTests` (5 new tests, all green). Full suite: 104 tests, 0 failures. + +### Client.CLI-012 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `docs/Client.CLI.md:264` | +| Status | Resolved | + +**Description:** `docs/Client.CLI.md` states "The Client CLI has 77 unit tests" but 99 tests existed at HEAD before this re-review, and 104 after the Client.CLI-011 fix. The count was not updated as new test classes were added after the first review. + +**Recommendation:** Update the test count in `docs/Client.CLI.md` to reflect the current 104 tests. + +**Resolution:** Resolved 2026-06-19 — updated test count in `docs/Client.CLI.md` from 77 to 104. diff --git a/docs/Client.CLI.md b/docs/Client.CLI.md index ab1f78dc..3d7f5a3d 100644 --- a/docs/Client.CLI.md +++ b/docs/Client.CLI.md @@ -261,7 +261,7 @@ Application URI: urn:localhost:OtOpcUa:instance1 ## Testing -The Client CLI has 77 unit tests covering option parsing, service invocation, output formatting, and cleanup behavior: +The Client CLI has 104 unit tests covering option parsing, service invocation, output formatting, and cleanup behavior: ```bash dotnet test tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AcknowledgeCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AcknowledgeCommand.cs index 6fb54813..0d685a81 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AcknowledgeCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/AcknowledgeCommand.cs @@ -2,6 +2,7 @@ using CliFx.Attributes; using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -43,6 +44,15 @@ public class AcknowledgeCommand : CommandBase { ConfigureLogging(); + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + byte[] eventId; try { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ConfirmCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ConfirmCommand.cs index 1865503c..da7893b9 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ConfirmCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ConfirmCommand.cs @@ -2,6 +2,7 @@ using CliFx.Attributes; using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -43,6 +44,15 @@ public class ConfirmCommand : CommandBase { ConfigureLogging(); + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + byte[] eventId; try { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/DisableCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/DisableCommand.cs index 68641697..a9619bbb 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/DisableCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/DisableCommand.cs @@ -1,6 +1,8 @@ using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -30,6 +32,15 @@ public class DisableCommand : CommandBase { ConfigureLogging(); + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/EnableCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/EnableCommand.cs index 2ecca347..20d3446e 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/EnableCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/EnableCommand.cs @@ -1,6 +1,8 @@ using CliFx.Attributes; +using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; @@ -30,6 +32,15 @@ public class EnableCommand : CommandBase { ConfigureLogging(); + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + IOpcUaClientService? service = null; try { diff --git a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ShelveCommand.cs b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ShelveCommand.cs index 0c03386d..752726ac 100644 --- a/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ShelveCommand.cs +++ b/src/Client/ZB.MOM.WW.OtOpcUa.Client.CLI/Commands/ShelveCommand.cs @@ -2,6 +2,7 @@ using CliFx.Attributes; using CliFx.Exceptions; using CliFx.Infrastructure; using Opc.Ua; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Helpers; using ZB.MOM.WW.OtOpcUa.Client.Shared; using ZB.MOM.WW.OtOpcUa.Client.Shared.Models; @@ -45,6 +46,15 @@ public class ShelveCommand : CommandBase { ConfigureLogging(); + try + { + NodeIdParser.ParseRequired(NodeId); + } + catch (Exception ex) when (ex is FormatException or ArgumentException) + { + throw new CommandException($"Invalid --node value: {ex.Message}"); + } + if (!Enum.TryParse(Kind, ignoreCase: true, out var shelveKind)) throw new CommandException( $"Invalid --kind value '{Kind}'. Expected one of: OneShot, Timed, Unshelve."); diff --git a/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AlarmOpCommandNodeIdValidationTests.cs b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AlarmOpCommandNodeIdValidationTests.cs new file mode 100644 index 00000000..780698b7 --- /dev/null +++ b/tests/Client/ZB.MOM.WW.OtOpcUa.Client.CLI.Tests/AlarmOpCommandNodeIdValidationTests.cs @@ -0,0 +1,108 @@ +using CliFx.Exceptions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Commands; +using ZB.MOM.WW.OtOpcUa.Client.CLI.Tests.Fakes; + +namespace ZB.MOM.WW.OtOpcUa.Client.CLI.Tests; + +/// +/// Regression tests for Client.CLI-011: alarm-op commands (ack, confirm, enable, disable, shelve) +/// must validate the --node option with a clean CommandException rather than letting a raw +/// exception propagate from NodeId.Parse inside the service. +/// +public class AlarmOpCommandNodeIdValidationTests +{ + private static string HexOf(byte[] bytes) => Convert.ToHexString(bytes); + + /// AcknowledgeCommand with an invalid node ID must throw CommandException, not a raw exception. + [Fact] + public async Task AcknowledgeCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new AcknowledgeCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id", + EventId = HexOf(new byte[] { 0x01 }), + Comment = "" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } + + /// ConfirmCommand with an invalid node ID must throw CommandException, not a raw exception. + [Fact] + public async Task ConfirmCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new ConfirmCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id", + EventId = HexOf(new byte[] { 0x01 }), + Comment = "" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } + + /// EnableCommand with an invalid node ID must throw CommandException, not a raw exception. + [Fact] + public async Task EnableCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new EnableCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } + + /// DisableCommand with an invalid node ID must throw CommandException, not a raw exception. + [Fact] + public async Task DisableCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new DisableCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id" + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } + + /// ShelveCommand with an invalid node ID must throw CommandException, not a raw exception. + [Fact] + public async Task ShelveCommand_InvalidNodeId_ThrowsCommandException() + { + var fakeService = new FakeOpcUaClientService(); + var factory = new FakeOpcUaClientServiceFactory(fakeService); + var command = new ShelveCommand(factory) + { + Url = "opc.tcp://localhost:4840", + NodeId = "not-a-node-id", + Kind = "OneShot", + DurationSeconds = 0 + }; + + using var console = TestConsoleHelper.CreateConsole(); + var ex = await Should.ThrowAsync(async () => await command.ExecuteAsync(console)); + ex.Message.ShouldContain("node", Case.Insensitive); + } +}