From 1180b017f505e177638c2476ea80678ec160f55d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:21:36 -0400 Subject: [PATCH] review(Driver.Cli.Common): drop dead FormatStatus branch + timestamp-kind test Re-review at 7286d320. -009: remove unreachable name-is-null branch in FormatStatus + invariant test. -010: pin DateTimeKind.Unspecified FormatTimestamp behavior. --- code-reviews/Driver.Cli.Common/findings.md | 105 +++++++++++++++++- .../SnapshotFormatter.cs | 6 +- .../SnapshotFormatterTests.cs | 33 ++++++ 3 files changed, 139 insertions(+), 5 deletions(-) diff --git a/code-reviews/Driver.Cli.Common/findings.md b/code-reviews/Driver.Cli.Common/findings.md index eed5d1da..cba886d7 100644 --- a/code-reviews/Driver.Cli.Common/findings.md +++ b/code-reviews/Driver.Cli.Common/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common` | | Reviewer | Claude Code | -| Review date | 2026-05-23 | -| Commit reviewed | `a9be809` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | @@ -362,3 +362,104 @@ well-known Theory comment block; the redundant Theory is dead weight. `InlineData` rows are covered by the well-known Theory's `ShouldBe` (strict exact-match assertion), which is the authoritative shortlist test. Landed alongside the Driver.Cli.Common-007 fix in the same commit. + +--- + +## Re-review 2026-06-19 (commit `7286d320`) + +Delta scope since `a9be809`: + +- `a6ae4e22` — `fix(status-codes)`: corrected `BadDeviceFailure` from + `0x80550000` to `0x808B0000` in `FormatStatus` and all six native-protocol + mappers (Driver.Cli.Common-007); deleted the redundant `FormatStatus_names_native_driver_emitted_codes` + Theory (Driver.Cli.Common-008). Both already Resolved. +- `2b811477` — `chore(build)`: stripped inline `Version` attributes from + `PackageReference` elements and centralised them in `Directory.Packages.props`. + No logic change. +- `64e3fbe0` — `docs`: XML documentation backfilled on all previously-undocumented + public members (``, ``, ``, `` where + applicable). No logic change; `CS1591` suppression in the csproj retained (the + `NoWarn` suppression now acts as a belt-and-suspenders guard rather than primary + silence). + +Review covered all 10 categories. Two new findings recorded (both Low). + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.Cli.Common-009 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 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 | Driver.Cli.Common-010 | +| 10 | Documentation & comments | No issues found | + +Both findings were fixed in-session (TDD: tests first, then src change). Suite +went from 43 → 49 tests; build clean. + +### Driver.Cli.Common-009 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:159-161` | +| Status | Resolved | + +**Description:** After the named-shortlist match and the severity-class fallback +that were introduced by Driver.Cli.Common-002, `name` can never be `null` at the +`return` statement — the severity fallback switch unconditionally assigns one of +`"Good"`, `"Uncertain"`, or `"Bad"`. The surviving `name is null` ternary therefore +has a dead branch: the bare-hex path (`$"0x{statusCode:X8}"` with no parenthesised +label) is unreachable. This is misleading to readers: it implies there exists a +code path where neither the named shortlist nor the severity fallback produce a +label, which is false. No correctness impact in practice, but it contradicts the +invariant the -002 fix established (operators always see a quality label) and could +confuse a future maintainer who extends the shortlist and thinks they need to handle +the `null` case explicitly. + +**Recommendation:** Remove the dead ternary; replace with a direct +`return $"0x{statusCode:X8} ({name})";` and add a clarifying comment that `name` +is always non-null at this point (assigned by shortlist or fallback). Pin a +regression `[Theory]` test asserting every output includes a `"("` so future edits +cannot accidentally reintroduce the bare-hex path. + +**Resolution:** Resolved 2026-06-19 — removed the dead `name is null` ternary in +`FormatStatus`; replaced with `return $"0x{statusCode:X8} ({name})";` plus a +comment explaining the invariant. Regression test +`FormatStatus_always_includes_parenthesised_label` (5-row `[Theory]`) added to +`SnapshotFormatterTests.cs` to pin the guarantee. + +### Driver.Cli.Common-010 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:167-172` / `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs` | +| Status | Resolved | + +**Description:** `FormatTimestamp` converts `DateTime` values whose `Kind` is not +`DateTimeKind.Utc` via `ToUniversalTime()`. For `DateTimeKind.Local` this is +correct and tested (`FormatTimestamp_normalises_local_kind_to_utc`). However, +`DateTimeKind.Unspecified` is a valid `Kind` produced by, e.g., EF-materialized +timestamps or unconfigured driver clocks; `ToUniversalTime()` treats `Unspecified` +identically to `Local` (documented .NET behavior). The test suite never exercises +this case. The behavior is correct per OPC UA conventions (unspecified → assume +local → convert to UTC), but the gap was noted in Driver.Cli.Common-005's description +and was never closed during the -005 resolution commit. Without the test, a future +refactor of the `Kind` branch could silently drop `Unspecified` conversion and no +test would catch it. + +**Recommendation:** Add a `[Fact]` asserting that a `DateTimeKind.Unspecified` +timestamp also produces a `"Z"`-suffixed output string, documenting the +"treat Unspecified as Local" contract. + +**Resolution:** Resolved 2026-06-19 — added +`FormatTimestamp_treats_unspecified_kind_as_local_and_converts_to_utc` to +`SnapshotFormatterTests.cs`, asserting that a `DateTimeKind.Unspecified` value +produces a `"Z"`-suffixed ISO-8601 string. No source change needed (behavior was +already correct). diff --git a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs index b44ffdc5..2f5c5654 100644 --- a/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs +++ b/src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs @@ -156,9 +156,9 @@ public static class SnapshotFormatter }; } - return name is null - ? $"0x{statusCode:X8}" - : $"0x{statusCode:X8} ({name})"; + // name is always non-null here: the named shortlist assigns it directly, and + // the severity-class fallback above assigns one of "Good" / "Uncertain" / "Bad". + return $"0x{statusCode:X8} ({name})"; } /// Formats a UTC timestamp as an ISO 8601 string, or "-" if null. diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs index dd73c2cd..55683379 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs @@ -217,6 +217,39 @@ public sealed class SnapshotFormatterTests table.ShouldContain("SOURCE TIME"); table.ShouldContain("---"); } + + // --- Driver.Cli.Common-009: dead-code in FormatStatus --- + + /// Verifies that FormatStatus always includes a parenthesised label — bare-hex-only output is unreachable. + /// An arbitrary OPC UA status code. + [Theory] + // The severity-class fallback fires for every code not in the named shortlist, so + // the output always carries a "(