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

review: regenerate code-review index after Batch 1 (OpcUaServer/Security/Host/Cluster/Commons)

Browse Source
This commit is contained in:
Joseph Doherty
2026-06-19 10:37:26 -04:00
parent bac6613dd2
commit 5aaa82bc26
1 changed files with 28 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/README.md
+28 -1
View File
@@ -15,6 +15,8 @@ Each module's `findings.md` is the source of truth; this file is generated from
| [Client.CLI](Client.CLI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 |
| [Client.Shared](Client.Shared/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
| [Client.UI](Client.UI/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
| [Cluster](Cluster/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 5 |
| [Commons](Commons/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 4 |
| [Configuration](Configuration/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 |
| [Core](Core/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 |
| [Core.Abstractions](Core.Abstractions/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 |
@@ -40,13 +42,22 @@ Each module's `findings.md` is the source of truth; this file is generated from
| [Driver.S7.Cli](Driver.S7.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
| [Driver.TwinCAT](Driver.TwinCAT/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 16 |
| [Driver.TwinCAT.Cli](Driver.TwinCAT.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 |
| [Host](Host/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 1 | 4 |
| [OpcUaServer](OpcUaServer/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 4 | 6 |
| [Security](Security/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 2 |
| [Server](Server/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 |
## Pending findings
Findings with status `Open` or `In Progress`, ordered by severity.
_No pending findings._
| ID | Severity | Category | Location | Description |
|---|---|---|---|---|
| OpcUaServer-001 | Medium | Correctness & logic bugs | `AddressSpacePlan.cs:56` (`AddressSpacePlan.IsEmpty`), `AddressSpacePlan.cs:80` (`AddressSpacePlanner.Compute`) | `AddressSpaceComposition` carries the UNS topology (`UnsAreas` + `UnsLines`), and `AddressSpaceApplier.MaterialiseHierarchy` uses each area's/line's `DisplayName` for the OPC UA folder display name. But `AddressSpacePlanner.Compute` only d… |
| OpcUaServer-002 | Medium | Correctness & logic bugs | `OtOpcUaNodeManager.cs:1748` (`HistoryReadEvents`), `OtOpcUaNodeManager.cs:1814` (`ClampToInt`) | For HistoryRead-Events, `HistoryReadEvents` passes `ClampToInt(details.NumValuesPerNode)` to `IHistorianDataSource.ReadEventsAsync(maxEvents)` and always returns the result with `ContinuationPoint = null` ("the full window in one shot"). T… |
| Host-003 | Low | Design-document adherence | `docs/ServiceHosting.md` (section "Per-role configuration overlays") | `docs/ServiceHosting.md` states the configuration loading order as "base `appsettings.json` → role overlay (`appsettings.{role}.json`) → environment overlay (`appsettings.{Environment}.json`) — later layers win." This is incorrect. The act… |
| OpcUaServer-003 | Low | Correctness & logic bugs | `OtOpcUaNodeManager.cs:1978` (`ServeRawPaged`), `HistoryPaging.cs` (whole), `HistoryPaging.cs:213` (`SliceTieCluster` `next <= endUtc`) | The Raw paging chain treats `endUtc` as an **inclusive** upper bound throughout — the `HistoryContinuationState`/`HistoryPaging` XML docs all say "the original (inclusive) end of the window", and `SliceTieCluster` advances with `next <= en… |
| OpcUaServer-004 | Low | Error handling & resilience | `OtOpcUaNodeManager.cs:1597` (`ResolveParentFolder`), and every public sink mutator that calls it (`EnsureFolder` 1278, `EnsureVariable` 1335, `MaterialiseAlarmCondition` 597, plus `WriteValue`/`WriteAlarmCondition` `CreateVariable`) | `ResolveParentFolder` dereferences `_root!` with the null-forgiving operator, and `CreateVariable` uses `_root` (`AddChild`). `_root` is only assigned in `CreateAddressSpace`, which the SDK invokes during `StandardServer` start. Every publ… |
## Closed findings
@@ -107,6 +118,8 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Driver.TwinCAT-007 | High | Resolved | Concurrency & thread safety | `TwinCATDriver.cs:413-429` |
| Driver.TwinCAT-008 | High | Resolved | Concurrency & thread safety | `AdsTwinCATClient.cs:162-169`, `TwinCATDriver.cs:319-324` |
| Driver.TwinCAT-013 | High | Resolved | Design-document adherence | `TwinCATDriver.cs:11-12` (capability list), whole file |
| Host-001 | High | Resolved | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Host/Observability/ObservabilityExtensions.cs:47`, `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs:245` |
| Security-001 | High | Resolved | Security | `src/Server/ZB.MOM.WW.OtOpcUa.Security/Endpoints/AuthEndpoints.cs:135` |
| Server-002 | High | Resolved | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs:60-63` |
| Server-009 | High | Resolved | Security | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs:44`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs:74` |
| Admin-006 | Medium | Resolved | Security | `Components/Layout/MainLayout.razor:47-49`, `Program.cs:129,131-135` |
@@ -126,6 +139,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Client.UI-005 | Medium | Resolved | Concurrency & thread safety | `ViewModels/MainWindowViewModel.cs:286-304`, `ViewModels/MainWindowViewModel.cs:155-189` |
| Client.UI-007 | Medium | Resolved | Security | `Services/UserSettings.cs:22-23`, `Services/JsonSettingsService.cs:38-50`, `ViewModels/MainWindowViewModel.cs:393-408` |
| Client.UI-008 | Medium | Resolved | Performance & resource management | `ViewModels/MainWindowViewModel.cs:18`, `ViewModels/MainWindowViewModel.cs:125-148`, `App.axaml.cs:18-32` |
| Cluster-001 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs:100-105` |
| Configuration-002 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:325` |
| Configuration-003 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs:73` |
| Configuration-006 | Medium | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/ResilientConfigReader.cs:79` |
@@ -233,6 +247,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Driver.TwinCAT-010 | Medium | Resolved | Error handling & resilience | `AdsTwinCATClient.cs:178-195` |
| Driver.TwinCAT-011 | Medium | Resolved | Error handling & resilience | `TwinCATStatusMapper.cs:29-42` |
| Driver.TwinCAT-012 | Medium | Resolved | Performance & resource management | `TwinCATDriver.cs:102`, `AdsTwinCATClient.cs:178-195` |
| Host-002 | Medium | Resolved | Security | `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs:163–180`, `src/Server/ZB.MOM.WW.OtOpcUa.Host/Configuration/LdapOptionsValidator.cs` |
| Server-003 | Medium | Resolved | Correctness & logic bugs | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Phase7/RingBufferHistoryWriter.cs:96-119` |
| Server-005 | Medium | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Alarms/AlarmConditionService.cs:166`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:303-311` |
| Server-007 | Medium | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OpcUaApplicationHost.cs:179-183` |
@@ -266,6 +281,14 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Client.UI-009 | Low | Resolved | Design-document adherence | `ViewModels/HistoryViewModel.cs:44-54` |
| Client.UI-010 | Low | Resolved | Code organization & conventions | `Controls/DateTimeRangePicker.axaml.cs:33-37`, `Controls/DateTimeRangePicker.axaml.cs:70-80` |
| Client.UI-011 | Low | Resolved | Documentation & comments | `Views/MainWindow.axaml:81`, `Services/JsonSettingsService.cs:11-15` |
| Cluster-002 | Low | Deferred | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs:110-133` |
| Cluster-003 | Low | Resolved | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/HoconLoader.cs:12-16` |
| Cluster-004 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Cluster/Resources/akka.conf:4,7` |
| Cluster-005 | Low | Resolved | Testing coverage | `tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/ServiceLevelCalculatorTests.cs` |
| Commons-001 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs:79` |
| Commons-002 | Low | Deferred | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` |
| Commons-003 | Low | Resolved | Testing coverage | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`, `DeferredServiceLevelPublisher.cs` |
| Commons-004 | Low | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs` |
| Configuration-004 | Low | Resolved | OtOpcUa conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Enums/NodePermissions.cs:8`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/OtOpcUaConfigDbContext.cs:417` |
| Configuration-005 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs:50` |
| Configuration-007 | Low | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Apply/GenerationApplier.cs:44` |
@@ -394,6 +417,10 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`.
| Driver.TwinCAT.Cli-005 | Low | Resolved | Code organization & conventions | `Commands/ProbeCommand.cs:23`, `Commands/ReadCommand.cs:20`, `Commands/WriteCommand.cs:20`, `Commands/SubscribeCommand.cs:18` |
| Driver.TwinCAT.Cli-006 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Cli.Tests/WriteCommandParseValueTests.cs` |
| Driver.TwinCAT.Cli-007 | Low | Resolved | Documentation & comments | `TwinCATCommandBase.cs:31-36` |
| Host-004 | Low | Resolved | Documentation & comments | `src/Server/ZB.MOM.WW.OtOpcUa.Host/Observability/ObservabilityExtensions.cs:41–42` |
| OpcUaServer-005 | Low | Won't Fix | Testing coverage | `OtOpcUaNodeManager.cs:2049` (`ServeRawPaged` tie-cluster stall path), `OtOpcUaNodeManager.cs:2068` (absurd-burst backstop) |
| OpcUaServer-006 | Low | Resolved | Documentation & comments | `OtOpcUaNodeManager.cs:11-30` (class XML doc), `OpcUaApplicationHost.cs:88-93` / `OpcUaApplicationHost.cs:421-423` (F13/F13c follow-up notes) |
| Security-002 | Low | Resolved | Documentation & comments | `src/Server/ZB.MOM.WW.OtOpcUa.Security/Ldap/LdapOptions.cs:9` |
| Server-004 | Low | Resolved | OtOpcUa conventions | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OtOpcUaServer.cs:187-200` |
| Server-006 | Low | Resolved | Concurrency & thread safety | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:478-482, 1342-1348` |
| Server-008 | Low | Resolved | Error handling & resilience | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/DriverNodeManager.cs:736` |