diff --git a/code-reviews/README.md b/code-reviews/README.md index e0762b69..1f5f30a7 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -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` |