dohertj2/lmxopcua
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

review(Core): re-review at HEAD; clean up duplicate/unexplained doc comments

Browse Source 
Re-review at 7286d320. Core-013 (duplicate <summary> on HostBoundHandle), Core-014
(clarify EquipmentNodeWalker test-only hardcoded attrs). Both Low, doc-only. Prior
authz/Galaxy churn verified correct.
This commit is contained in:
Joseph Doherty
2026-06-19 11:06:56 -04:00
parent 8ac5a2dbc5
commit 354b0e7613
3 changed files with 68 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Core/findings.md
+62 -2
View File
@@ -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 `<summary>` 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` `<remarks>` state matrix and state it maps to Degraded.
**Resolution:** Resolved 2026-05-23 — replaced the `WedgeDetector(.ctor)` `<summary>` with an accurate "Construct with the wedge-detection threshold; values below 60 s clamp to 60 s" description plus a `<param>` block; added the `Reconnecting` row to the `DriverHealthReport` `<remarks>` 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 `<summary>` XML doc blocks (lines 153-157 and 158-162). The C# compiler tolerates this but the generated XML doc contains a duplicate `<member>` 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 `<summary>` block, keeping exactly one.
**Resolution:** Resolved 2026-06-19 — removed the second duplicate `<summary>` 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).