dohertj2/lmxopcua
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

review(Client.CLI): wrap NodeId parse errors in CommandException for alarm-op commands

Browse Source 
Re-review at 7286d320. -011: ack/confirm/enable/disable/shelve now pre-validate --node and
surface a clean CommandException (was a raw FormatException) + tests. -012: refresh stale
test count in docs/Client.CLI.md.
This commit is contained in:
Joseph Doherty
2026-06-19 11:58:15 -04:00
parent cd072baad8
commit 887a31e825
8 changed files with 210 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Client.CLI/findings.md
+49 -2
View File
@@ -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.