diff --git a/code-reviews/Core/findings.md b/code-reviews/Core/findings.md index b0269701..a9c93c57 100644 --- a/code-reviews/Core/findings.md +++ b/code-reviews/Core/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | @@ -205,3 +205,63 @@ **Recommendation:** Replace the `WedgeDetector` constructor `` with an accurate description (e.g. "Construct with the wedge-detection threshold; values below 60 s clamp to 60 s"). Add `Reconnecting` to the `DriverHealthReport` `` state matrix and state it maps to Degraded. **Resolution:** Resolved 2026-05-23 — replaced the `WedgeDetector(.ctor)` `` with an accurate "Construct with the wedge-detection threshold; values below 60 s clamp to 60 s" description plus a `` block; added the `Reconnecting` row to the `DriverHealthReport` `` state matrix and updated the verdict-rule prose. Added `WedgeDetectorTests.Doc_Constructor_Summary_Describes_Threshold_Clamp` and `DriverHealthReportTests.Doc_State_Matrix_Includes_Reconnecting` regression tests that parse the generated `.xml` doc to assert the strings, plus `Any_Reconnecting_WithoutFaultedOrNotReady_IsDegraded` confirming the documented Reconnecting → Degraded behaviour. + +--- + +## Re-review 2026-06-19 (commit 7286d320) + +#### What changed since 76d35d1 + +- **Galaxy standard-driver migration (Phase A):** `NodeHierarchyKind.SystemPlatform` and `NodeScope.FolderSegments` removed; `PermissionTrie.WalkSystemPlatform` deleted; `CollectMatches` now unconditionally delegates to `WalkEquipment`. Galaxy points resolve as ordinary Equipment-kind tags through the same UNS walk. +- **`HistoryUpdate` operation mapping fixed:** `TriePermissionEvaluator.MapOperationToPermission` previously mapped `OpcUaOperation.HistoryUpdate → NodePermissions.HistoryRead`; now correctly maps to `NodePermissions.HistoryUpdate`. The prior TODO marker is gone; `NodePermissions.HistoryUpdate` is a real bit (1 << 12) with tests asserting no collision with `MethodCall`. +- **Generation-stamp guard added** to `TriePermissionEvaluator.Authorize` (Core-002 full fix): compares `trie.GenerationId` to `session.AuthGenerationId` and re-fetches the bound generation, failing closed when pruned. +- **`HostBoundHandle` per-host routing** in `AlarmSurfaceInvoker` (Core-007 full fix): `SubscribeAsync` wraps each driver handle with resolved host; `UnsubscribeAsync` unwraps it. +- **`CapabilityInvoker.ExecuteWriteAsync`** now snapshots `_optionsAccessor()` once per non-idempotent write (Core-009 fix). +- **XML doc coverage** added throughout (constructors, params, returns, inner classes). +- **`EquipmentNodeWalker`** updated to reflect Galaxy-standard-driver retirement of SystemPlatform path; `EquipmentNamespaceContent` doc trimmed to remove stale generation-scoping language. +- **`DriverResilienceOptions.Resolve`** throws a diagnostic `KeyNotFoundException` instead of crashing with `KeyNotFoundException` from dict indexer (Core-010 fix). + +#### Checklist coverage (re-review) + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No new issues | +| 2 | OtOpcUa conventions | No new issues | +| 3 | Concurrency & thread safety | No new issues | +| 4 | Error handling & resilience | No new issues | +| 5 | Security | No new issues — HistoryUpdate mapping fix verified correct; generation-stamp guard correct | +| 6 | Performance & resource management | No new issues | +| 7 | Design-document adherence | No new issues | +| 8 | Code organization & conventions | Core-013 | +| 9 | Testing coverage | No new gaps | +| 10 | Documentation & comments | Core-013, Core-014 | + +### Core-013 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments / Code organization | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs:153-162` | +| Status | Resolved | + +**Description:** The Core-007 fix introduced a `HostBoundHandle` private class with two consecutive identical `` XML doc blocks (lines 153-157 and 158-162). The C# compiler tolerates this but the generated XML doc contains a duplicate `` entry for the type. Consumers of the XML (IDEs, `dotnet-xml-doc`) surface the duplicate as a warning or silently drop the second entry. + +**Recommendation:** Remove the duplicate `` block, keeping exactly one. + +**Resolution:** Resolved 2026-06-19 — removed the second duplicate `` block from `HostBoundHandle`; single-block doc retained. No regression test needed: the build verifies zero XML-doc warnings and the AlarmSurfaceInvokerTests suite (8 tests) covers the class behaviour. + +### Core-014 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs:158-168` | +| Status | Resolved | + +**Description:** `EquipmentNodeWalker.AddTagVariable` hardcodes `SecurityClass: SecurityClassification.FreeAccess` and `IsHistorized: false` without any comment explaining why these fields are not read from `TagConfig`. A maintainer unfamiliar with the retirement history (the production server path moved to `AddressSpaceComposer → AddressSpaceApplier → Sink` in ADR-001) could incorrectly assume the walker is the live composition path and spend time debugging missing `HistoryRead` bits or wrong security classes on nodes. In production, `DeploymentArtifact.ExtractTagHistorize` reads `isHistorized`/`historianTagname` from `TagConfig` JSON; `EquipmentNodeWalker.Walk` is only called by unit tests. + +**Recommendation:** Add an inline comment above the `DriverAttributeInfo` constructor call explaining that the walker is test-support only in production; real nodes use `AddressSpaceComposer/DeploymentArtifact`. + +**Resolution:** Resolved 2026-06-19 — added an 8-line inline comment above the `DriverAttributeInfo` constructor in `AddTagVariable` documenting the deliberate omission and naming the production code path. No behaviour change; no regression test needed (comment only). diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs index 7fdb0126..58ea16e8 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs @@ -157,6 +157,12 @@ public static class EquipmentNodeWalker /// private static void AddTagVariable(IAddressSpaceBuilder equipmentBuilder, Tag tag) { + // SecurityClass and IsHistorized are intentionally not extracted from TagConfig here. + // In production, EquipmentNodeWalker.Walk is superseded by the + // AddressSpaceComposer → AddressSpaceApplier → Sink → NodeManager chain, which reads + // both fields from TagConfig JSON directly (via DeploymentArtifact.ExtractTagHistorize + // and the SecurityClassification column). This walker is retained for unit-test support + // only; real server deployments never invoke Walk to build live nodes. var attr = new DriverAttributeInfo( FullName: ExtractFullName(tag.TagConfig), DriverDataType: ParseDriverDataType(tag.DataType), diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs index ba2c6014..c2e2c24a 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs @@ -150,11 +150,6 @@ public sealed class AlarmSurfaceInvoker return result; } - /// - /// Wraps an returned by the driver with the - /// resolved host name used when the subscription was created. - /// unwraps this to route the unsubscribe through the same host's resilience pipeline. - /// /// /// Wraps an returned by the driver with the /// resolved host name used when the subscription was created.