review(Commons): record findings + add deferred-sink/equip-nodeid tests, fix stale Phase7 doc

Code review at HEAD 7286d320. Commons-001 (stale Phase7 telemetry doc) fixed; Commons-003/004 close test-coverage gaps (DeferredAddressSpaceSink/ServiceLevelPublisher forwarding seam + EquipmentNodeIds whitespace branch). Commons-002 (CorrelationId typing) deferred as cross-cutting.
2026-06-19 10:22:59 -04:00
parent 7286d3208a
commit 6dc74289ce
5 changed files with 332 additions and 1 deletions
@@ -0,0 +1,120 @@
+# Code Review — Commons
+
+<!-- Per-module findings for the Commons shared-utilities / message-contracts library.
+     See ../../REVIEW-PROCESS.md for the full process. The base README.md is generated
+     from these files by regen-readme.py — do not edit README.md by hand. -->
+
+| Field | Value |
+|---|---|
+| Module | `src/Core/ZB.MOM.WW.OtOpcUa.Commons` |
+| Reviewer | Claude Code |
+| Review date | 2026-06-19 |
+| Commit reviewed | `7286d320` |
+| Status | Reviewed |
+| Open findings | 0 |
+
+## Checklist coverage
+
+A comprehensive review completes every category, recording "No issues found" where
+a category produced nothing rather than leaving it blank.
+
+| # | Category | Result |
+|---|---|---|
+| 1 | Correctness & logic bugs | No issues found |
+| 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 | Commons-002 (deferred) |
+| 9 | Testing coverage | Commons-003, Commons-004 |
+| 10 | Documentation & comments | Commons-001 |
+
+## Findings
+
+### Commons-001
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs:79` |
+| Status | Resolved |
+
+**Description:** The XML doc comment on `StartAddressSpaceRebuildSpan` read "Phase7 plan →
+apply", referencing the old `Phase7*` naming that was renamed to `AddressSpace*` in commit
+`40e8a23e` (2026-06-18). The method name itself was correct; only the summary text was stale.
+
+**Recommendation:** Replace "Phase7 plan → apply" with "AddressSpace plan → apply" in the summary.
+
+**Resolution:** Fixed 2026-06-19 — updated stale "Phase7 plan → apply" to "AddressSpace plan → apply" in the XML doc comment (src edit, no commit SHA yet).
+
+---
+
+### Commons-002
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Code organization & conventions |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` |
+| Status | Deferred |
+
+**Description:** `RestartDriver`, `ReconnectDriver`, and `TestDriverConnect` (plus their result
+records) carry `Guid CorrelationId` parameters using the raw `System.Guid` type, while every
+other Commons message record uses the typed `CorrelationId` wrapper. This divergence requires
+callers to manually extract `.Value` or wrap a raw `Guid` when correlating across these
+boundaries. The inconsistency appears to be a historical accident — these three messages
+predate the `CorrelationId` struct.
+
+**Recommendation:** Migrate `Guid CorrelationId` → `CorrelationId CorrelationId` across
+`RestartDriver`, `ReconnectDriver`, `TestDriverConnect` and their result records. This is a
+cross-cutting public-API change that touches callers in at least AdminUI, Runtime, and Cluster
+projects; defer to a coordinated cleanup pass.
+
+**Resolution:** Deferred — cross-cutting public-API change requiring coordinated update across at least three other modules (AdminUI, Runtime, Cluster). Safe to defer.
+
+---
+
+### Commons-003
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Testing coverage |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`, `DeferredServiceLevelPublisher.cs` |
+| Status | Resolved |
+
+**Description:** `DeferredAddressSpaceSink` and `DeferredServiceLevelPublisher` had no unit
+tests. The deferred-sink pattern is a production-critical seam (a previously recorded MEMORY.md
+gotcha: "a new optional capability interface MUST be forwarded through
+`DeferredAddressSpaceSink` or it's inert on every driver-role host"). Key invariants now
+covered: (1) before `SetSink`, all write operations are safe no-ops; (2) after `SetSink`,
+operations are forwarded; (3) `UpdateTagAttributes` returns `false` for non-surgical inners and
+`true` for surgical inners; (4) `SetSink(null)` / `SetInner(null)` reverts to the null sink.
+
+**Recommendation:** Add `DeferredAddressSpaceSinkTests` and `DeferredServiceLevelPublisherTests`
+under `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/`.
+
+**Resolution:** Fixed 2026-06-19 — added `DeferredAddressSpaceSinkTests.cs` (8 tests) and `DeferredServiceLevelPublisherTests.cs` (3 tests) covering the five invariants; all pass.
+
+---
+
+### Commons-004
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Testing coverage |
+| Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs` |
+| Status | Resolved |
+
+**Description:** `EquipmentNodeIds.Variable` uses `IsNullOrWhiteSpace` to detect empty
+`folderPath`, so a whitespace-only value (e.g. `"   "`) is treated identically to
+null/empty — the parent falls back to `equipmentId` directly. This is the correct behaviour
+but the whitespace-only branch had no test, leaving the intent implicit.
+
+**Recommendation:** Add an `[InlineData]` case covering whitespace-only `folderPath`.
+
+**Resolution:** Fixed 2026-06-19 — added `Variable_with_whitespace_folder_is_equipment_slash_name` test to `EquipmentNodeIdsTests.cs`; passes.
@@ -76,7 +76,7 @@ public static class OtOpcUaTelemetry
        return activity;
    }

-    /// <summary>Span wrapping a full OPC UA address-space rebuild (Phase7 plan → apply).</summary>
+    /// <summary>Span wrapping a full OPC UA address-space rebuild (AddressSpace plan → apply).</summary>
    public static Activity? StartAddressSpaceRebuildSpan()
        => ActivitySource.StartActivity("otopcua.opcua.address_space_rebuild", ActivityKind.Internal);
 }
@@ -0,0 +1,154 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
+
+namespace ZB.MOM.WW.OtOpcUa.Commons.Tests.OpcUa;
+
+/// <summary>
+/// Covers the deferred-sink forwarding invariants documented in Commons-003.
+/// The DeferredAddressSpaceSink is a production-critical seam: a new optional capability
+/// interface MUST be forwarded through it or the optimization is inert on every driver-role
+/// host (DeferredAddressSpaceSink is what actors inject, not the inner sink directly).
+/// </summary>
+public class DeferredAddressSpaceSinkTests
+{
+    // ---------- before SetSink — all calls are safe no-ops ----------
+
+    [Fact]
+    public void Before_SetSink_WriteValue_is_a_noop()
+    {
+        var sink = new DeferredAddressSpaceSink();
+        // Must not throw.
+        sink.WriteValue("ns=2;s=x", 42, OpcUaQuality.Good, DateTime.UtcNow);
+    }
+
+    [Fact]
+    public void Before_SetSink_RebuildAddressSpace_is_a_noop()
+    {
+        var sink = new DeferredAddressSpaceSink();
+        sink.RebuildAddressSpace(); // Must not throw.
+    }
+
+    [Fact]
+    public void Before_SetSink_UpdateTagAttributes_returns_false()
+    {
+        var sink = new DeferredAddressSpaceSink();
+        sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null,
+            dataType: "Boolean", isArray: false, arrayLength: null).ShouldBeFalse();
+    }
+
+    // ---------- after SetSink — operations are forwarded ----------
+
+    [Fact]
+    public void After_SetSink_WriteValue_is_forwarded()
+    {
+        var inner = new SpySink();
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(inner);
+
+        sink.WriteValue("ns=2;s=x", 99, OpcUaQuality.Good, DateTime.UtcNow);
+
+        inner.WriteValueCalled.ShouldBeTrue();
+    }
+
+    [Fact]
+    public void After_SetSink_RebuildAddressSpace_is_forwarded()
+    {
+        var inner = new SpySink();
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(inner);
+
+        sink.RebuildAddressSpace();
+
+        inner.RebuildCalled.ShouldBeTrue();
+    }
+
+    // ---------- ISurgicalAddressSpaceSink forwarding ----------
+
+    [Fact]
+    public void UpdateTagAttributes_returns_false_for_non_surgical_inner()
+    {
+        // SpySink does NOT implement ISurgicalAddressSpaceSink.
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(new SpySink());
+
+        sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null,
+            dataType: "Int32", isArray: false, arrayLength: null).ShouldBeFalse();
+    }
+
+    [Fact]
+    public void UpdateTagAttributes_returns_true_for_surgical_inner()
+    {
+        var surgical = new SpySurgicalSink();
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(surgical);
+
+        var result = sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null,
+            dataType: "Float", isArray: false, arrayLength: null);
+
+        result.ShouldBeTrue();
+        surgical.UpdateCalled.ShouldBeTrue();
+    }
+
+    // ---------- SetSink(null) reverts to null sink ----------
+
+    [Fact]
+    public void SetSink_null_reverts_to_null_sink_and_UpdateTagAttributes_returns_false()
+    {
+        var surgical = new SpySurgicalSink();
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(surgical);
+        sink.SetSink(null); // revert
+
+        sink.UpdateTagAttributes("ns=2;s=x", writable: false, historianTagname: null,
+            dataType: "Boolean", isArray: false, arrayLength: null).ShouldBeFalse();
+    }
+
+    [Fact]
+    public void SetSink_null_reverts_to_null_sink_and_WriteValue_is_noop()
+    {
+        var inner = new SpySink();
+        var sink = new DeferredAddressSpaceSink();
+        sink.SetSink(inner);
+        sink.SetSink(null); // revert
+
+        sink.WriteValue("ns=2;s=x", 1, OpcUaQuality.Good, DateTime.UtcNow);
+
+        inner.WriteValueCalled.ShouldBeFalse("write should be no-op after reverting to null sink");
+    }
+
+    // ---- test doubles ----
+
+    private sealed class SpySink : IOpcUaAddressSpaceSink
+    {
+        public bool WriteValueCalled { get; private set; }
+        public bool RebuildCalled { get; private set; }
+
+        public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc)
+            => WriteValueCalled = true;
+
+        public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc) { }
+        public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative = false) { }
+        public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName) { }
+        public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { }
+        public void RebuildAddressSpace() => RebuildCalled = true;
+    }
+
+    private sealed class SpySurgicalSink : IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink
+    {
+        public bool UpdateCalled { get; private set; }
+
+        public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) { }
+        public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc) { }
+        public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative = false) { }
+        public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName) { }
+        public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { }
+        public void RebuildAddressSpace() { }
+
+        public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength)
+        {
+            UpdateCalled = true;
+            return true;
+        }
+    }
+}
@@ -0,0 +1,53 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
+
+namespace ZB.MOM.WW.OtOpcUa.Commons.Tests.OpcUa;
+
+/// <summary>
+/// Covers DeferredServiceLevelPublisher forwarding invariants (Commons-003 companion).
+/// </summary>
+public class DeferredServiceLevelPublisherTests
+{
+    [Fact]
+    public void Before_SetInner_Publish_is_a_noop()
+    {
+        var publisher = new DeferredServiceLevelPublisher();
+        // Must not throw; the NullServiceLevelPublisher absorbs it.
+        publisher.Publish(200);
+    }
+
+    [Fact]
+    public void After_SetInner_Publish_is_forwarded()
+    {
+        var spy = new SpyPublisher();
+        var publisher = new DeferredServiceLevelPublisher();
+        publisher.SetInner(spy);
+
+        publisher.Publish(240);
+
+        spy.LastPublished.ShouldBe((byte)240);
+    }
+
+    [Fact]
+    public void SetInner_null_reverts_to_null_publisher()
+    {
+        var spy = new SpyPublisher();
+        var publisher = new DeferredServiceLevelPublisher();
+        publisher.SetInner(spy);
+        publisher.SetInner(null); // revert
+
+        publisher.Publish(100);
+
+        // The spy should NOT have received the publish after revert.
+        spy.LastPublished.ShouldBe((byte)0, "spy should not have been updated after revert to null");
+    }
+
+    // ---- test double ----
+
+    private sealed class SpyPublisher : IServiceLevelPublisher
+    {
+        public byte LastPublished { get; private set; }
+        public void Publish(byte serviceLevel) => LastPublished = serviceLevel;
+    }
+}
@@ -18,6 +18,10 @@ public class EquipmentNodeIdsTests
    public void Variable_with_folder_is_equipment_slash_folder_slash_name()
        => EquipmentNodeIds.Variable("eq-1", "registers", "speed").ShouldBe("eq-1/registers/speed");

+    [Fact]
+    public void Variable_with_whitespace_folder_is_equipment_slash_name()
+        => EquipmentNodeIds.Variable("eq-1", "   ", "speed").ShouldBe("eq-1/speed");
+
    [Fact]
    public void SubFolder_is_equipment_slash_folder()
        => EquipmentNodeIds.SubFolder("eq-1", "registers").ShouldBe("eq-1/registers");