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

@@ -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 (`<summary>`, `<param>`, `<returns>`, `<inheritdoc/>` 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).

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})";
     }
 
     /// <summary>Formats a UTC timestamp as an ISO 8601 string, or "-" if null.</summary>

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 ---
+
+    /// <summary>Verifies that FormatStatus always includes a parenthesised label — bare-hex-only output is unreachable.</summary>
+    /// <param name="statusCode">An arbitrary OPC UA status code.</param>
+    [Theory]
+    // The severity-class fallback fires for every code not in the named shortlist, so
+    // the output always carries a "(<label>)" suffix.  Driver.Cli.Common-009 noted that
+    // the "name is null" branch after the fallback is dead code; this Theory pins the
+    // guarantee so the dead branch can be safely removed.
+    [InlineData(0x00000000u)]   // Good
+    [InlineData(0x80000000u)]   // Bad
+    [InlineData(0x40000000u)]   // Uncertain
+    [InlineData(0x80990000u)]   // Unknown-bad → fallback "Bad"
+    [InlineData(0xDEADBEEFu)]  // Garbage → fallback "Bad"
+    public void FormatStatus_always_includes_parenthesised_label(uint statusCode)
+    {
+        // Verify the output contains "(" — meaning the name/severity label is always appended.
+        SnapshotFormatter.FormatStatus(statusCode).ShouldContain("(");
+    }
+
+    // --- Driver.Cli.Common-010: FormatTimestamp with Unspecified kind ---
+
+    /// <summary>Verifies that FormatTimestamp treats Unspecified kind as Local and converts to UTC.</summary>
+    [Fact]
+    public void FormatTimestamp_treats_unspecified_kind_as_local_and_converts_to_utc()
+    {
+        // DateTimeKind.Unspecified is silently treated as Local by ToUniversalTime().
+        // The result must still carry the "Z" suffix so consumers treat it as UTC.
+        var unspecified = new DateTime(2026, 4, 21, 12, 0, 0, DateTimeKind.Unspecified);
+        var formatted = SnapshotFormatter.FormatTimestamp(unspecified);
+        formatted.ShouldEndWith("Z");
+    }
 }
 
 [Trait("Category", "Unit")]