Joseph Doherty
5.4 KiB
Code Review — Commons
| 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 | Won't Fix |
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: Won't Fix (2026-10-08, #625) — accepted divergence. The three messages (RestartDriver, ReconnectDriver, TestDriverConnect) are admin-command round-trips whose Guid is generated and matched at the sender; migrating to the CorrelationId wrapper is a public-API change across AdminUI, Runtime and Cluster for no behavioural gain. Revisit only if one of those senders is being rewritten anyway.
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.