diff --git a/code-reviews/README.md b/code-reviews/README.md index 52e0dbc1..c3914ed0 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -18,13 +18,14 @@ Each module's `findings.md` is the source of truth; this file is generated from | [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 | +| [Configuration](Configuration/findings.md) | Claude Code | 2026-06-19 (re-review; first reviewed 2026-05-22) | `7286d320` (re-review; was `76d35d1`) | Reviewed | 2 | 14 | | [ControlPlane](ControlPlane/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 3 | -| [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 | +| [Core](Core/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 14 | +| [Core.Abstractions](Core.Abstractions/findings.md) | Claude Code | 2026-06-19 (re-review; original 2026-05-22) | `7286d320` | Reviewed | 0 | 10 | | [Core.AlarmHistorian](Core.AlarmHistorian/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 | | [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 13 | -| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 16 | +| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-06-19 | `7286d320` (re-review; HEAD at time of fix `8ac5a2db`) | Reviewed | 0 | 18 | +| [Core.Scripting.Abstractions](Core.Scripting.Abstractions/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 2 | 7 | | [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 | | [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | @@ -59,9 +60,13 @@ Findings with status `Open` or `In Progress`, ordered by severity. | ID | Severity | Category | Location | Description | |---|---|---|---|---| | AdminUI-001 | Medium | Security | `Components/Pages/ScriptEdit.razor:5`, `Components/Pages/Scripts.razor` | The Script CRUD surface (`/scripts/new`, `/scripts/{id}`, and the `/scripts` list) is gated by only `[Authorize]` (any authenticated user), whereas the *same editor's* Roslyn-analysis backend (`ScriptAnalysisEndpoints`) requires the `Fleet… | +| Configuration-013 | Medium | Design-document adherence | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs:243` (`ValidateClusterTopology`) | `DraftValidator.ValidateClusterTopology` is documented as the managed pre-publish guard that catches cluster-topology drift the SQL `CK_ServerCluster_RedundancyMode_NodeCount` check cannot see — specifically an operator disabling a `Cluste… | | 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… | | AdminUI-004 | Low | Security | `Hubs/FleetStatusHub.cs`, `Hubs/AlertHub.cs`, `Hubs/ScriptLogHub.cs` (vs `Hubs/DriverStatusHub.cs:12`) | Of the four SignalR hubs mapped by `MapOtOpcUaHubs`, only `DriverStatusHub` carries an explicit `[Authorize]` attribute. `FleetStatusHub`, `AlertHub`, and `ScriptLogHub` (which broadcast fleet status, alarm transitions with equipment paths… | +| Configuration-014 | Low | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Services/ILdapGroupRoleMappingService.cs:23`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Services/LdapGroupRoleMappingService.cs:24` | `ILdapGroupRoleMappingService.GetByGroupsAsync`'s XML doc asserts "Case-insensitive per LDAP conventions", but the implementation is `db.LdapGroupRoleMappings.Where(m => groupSet.Contains(m.LdapGroup))`, which translates to a SQL `IN (…)`… | +| Core.Scripting.Abstractions-004 | Low | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:84` | `ScriptContext.Deadband` has no guard or documentation for invalid `tolerance` values. A negative `tolerance` makes `Math.Abs(current - previous) > tolerance` trivially `true` for all finite inputs (any absolute value exceeds a negative nu… | +| Core.Scripting.Abstractions-007 | Low | Design-document adherence | `docs/ScriptedAlarms.md:298` | `docs/ScriptedAlarms.md` line 298 lists the file path as `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/AlarmPredicateContext.cs`. The file was moved to `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs` as… | | Driver.Galaxy.Browser-003 | Low | Code organization & conventions | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Browser/GalaxyDriverBrowser.cs:149` | `GalaxyDriverBrowser.ResolveApiKey` is a verbatim copy of `GalaxyDriver.ResolveApiKey`. The comment acknowledges this and explains why the Browser project intentionally does not reference Driver.Galaxy. However, Finding Driver.Galaxy.Brows… | | 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… | @@ -87,6 +92,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Client.Shared-006 | High | Resolved | Concurrency & thread safety | `OpcUaClientService.cs:97-100`, `OpcUaClientService.cs:432-497` | | Configuration-001 | High | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:282` | | Configuration-008 | High | Resolved | Security | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:150`, `:373`, `:468` | +| Configuration-012 | High | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/LiteDbConfigCache.cs:31`, `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/LocalCache/GenerationSealedCache.cs:71`, `:129` | | Core-001 | High | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/UserAuthorizationState.cs:50-68` | | Core-002 | High | Resolved | Security | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/TriePermissionEvaluator.cs:24-50` | | Core.AlarmHistorian-002 | High | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:99-105,386-388` | @@ -178,6 +184,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.Scripting-013 | Medium | Resolved | Security | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | | Core.Scripting-014 | Medium | Resolved | Concurrency & thread safety | `CompiledScriptCache.cs:91-103` (`Clear`) | | Core.Scripting-016 | Medium | Resolved | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:74-117`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:139-182` | +| Core.Scripting-017 | Medium | Resolved | Security | `ForbiddenTypeAnalyzer.cs:110-153` (`ForbiddenFullTypeNames`) | | Core.VirtualTags-002 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:237` | | Core.VirtualTags-003 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:117-120` | | Core.VirtualTags-005 | Medium | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs:50-64` | @@ -321,11 +328,15 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core-010 | Low | Resolved | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/DriverResilienceOptions.cs:45-52` | | Core-011 | Low | Resolved | Testing coverage | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs:58-75` | | Core-012 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Stability/WedgeDetector.cs:26`, `src/Core/ZB.MOM.WW.OtOpcUa.Core/Observability/DriverHealthReport.cs:11-22` | +| Core-013 | Low | Resolved | Documentation & comments / Code organization | `src/Core/ZB.MOM.WW.OtOpcUa.Core/Resilience/AlarmSurfaceInvoker.cs:153-162` | +| Core-014 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core/OpcUa/EquipmentNodeWalker.cs:158-168` | | Core.Abstractions-004 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverTypeRegistry.cs:23-40` | | Core.Abstractions-005 | Low | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/PollGroupEngine.cs:90,99` | | Core.Abstractions-006 | Low | Resolved | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs:63,84-86`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs:30,63` | | Core.Abstractions-007 | Low | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs` | | Core.Abstractions-008 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/DriverHealth.cs:9`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs:39-43,65-69` | +| Core.Abstractions-009 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs:89-107`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs:91-101` | +| Core.Abstractions-010 | Low | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs:68,111,128,246,280` | | Core.AlarmHistorian-008 | Low | Resolved | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:107-127,255-278` | | Core.AlarmHistorian-011 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/IAlarmHistorianSink.cs:5-9,76`, `AlarmHistorianEvent.cs:20` | | Core.ScriptedAlarms-003 | Low | Resolved | Documentation & comments | `ScriptedAlarmEngine.cs:343`, `docs/ScriptedAlarms.md:107` | @@ -341,6 +352,12 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.Scripting-009 | Low | Resolved | Design-document adherence | `ForbiddenTypeAnalyzer.cs:45` | | Core.Scripting-011 | Low | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/` | | Core.Scripting-015 | Low | Resolved | Correctness & logic bugs | `ScriptEvaluator.cs:234-270` (`ToCSharpTypeName`) | +| Core.Scripting-018 | Low | Resolved | Documentation & comments | `ForbiddenTypeAnalyzer.cs` (`CheckSymbol` rejection message) | +| Core.Scripting.Abstractions-001 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs:43,46` | +| Core.Scripting.Abstractions-002 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs:14` | +| Core.Scripting.Abstractions-003 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:9,10` | +| Core.Scripting.Abstractions-005 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs:21` | +| Core.Scripting.Abstractions-006 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:35-36,55-58` | | Core.VirtualTags-004 | Low | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:349` | | Core.VirtualTags-006 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:177-182`, `:395-401` | | Core.VirtualTags-007 | Low | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs:58` |