diff --git a/code-reviews/Core.VirtualTags/findings.md b/code-reviews/Core.VirtualTags/findings.md index 39c272c2..9cd5fa25 100644 --- a/code-reviews/Core.VirtualTags/findings.md +++ b/code-reviews/Core.VirtualTags/findings.md @@ -364,7 +364,7 @@ path) rather than rendering arrows, or reconstruct an actual cycle path within t All 13 prior findings remain Resolved (no regressions found). Three new findings were added. -### Re-review checklist +#### Re-review checklist | # | Category | Result | |---|---|---| diff --git a/code-reviews/README.md b/code-reviews/README.md index c3914ed0..84263d04 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -12,7 +12,7 @@ Each module's `findings.md` is the source of truth; this file is generated from |---|---|---|---|---|---|---| | [Admin](Admin/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [AdminUI](AdminUI/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 2 | 8 | -| [Analyzers](Analyzers/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 | +| [Analyzers](Analyzers/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 8 | | [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 | @@ -22,16 +22,16 @@ Each module's `findings.md` is the source of truth; this file is generated from | [ControlPlane](ControlPlane/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 3 | | [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.AlarmHistorian](Core.AlarmHistorian/findings.md) | Claude Code | 2026-06-19 | `621d00e4` | Reviewed | 1 | 14 | +| [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 15 | | [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 | +| [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 16 | | [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 | | [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 | -| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 8 | +| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-06-19 | `7286d320` | Reviewed | 0 | 10 | | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 | | [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 18 | @@ -65,6 +65,7 @@ Findings with status `Open` or `In Progress`, ordered by severity. | 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.AlarmHistorian-014 | Low | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:515-555,558-568` | `GetStatus()` and `RetryDeadLettered()` have no `_disposed` guard. After `Dispose()` is called both methods still open new `SqliteConnection` objects against the database file (which persists on disk after disposal). This works because SQL… | | 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… | @@ -172,6 +173,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.AlarmHistorian-007 | Medium | Resolved | Error handling & resilience | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:172-174` | | Core.AlarmHistorian-009 | Medium | Resolved | Design-document adherence | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:317-347` | | Core.AlarmHistorian-010 | Medium | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian.Tests/SqliteStoreAndForwardSinkTests.cs` | +| Core.AlarmHistorian-012 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:412-415` | | Core.ScriptedAlarms-002 | Medium | Resolved | Correctness & logic bugs | `ScriptedAlarmEngine.cs:162`, `ScriptedAlarmEngine.cs:90` | | Core.ScriptedAlarms-004 | Medium | Resolved | Concurrency & thread safety | `ScriptedAlarmEngine.cs:138-143`, `ScriptedAlarmEngine.cs:227-234` | | Core.ScriptedAlarms-005 | Medium | Resolved | Concurrency & thread safety | `ScriptedAlarmEngine.cs:365-369`, `ScriptedAlarmEngine.cs:416-424` | @@ -190,6 +192,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.VirtualTags-005 | Medium | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagSource.cs:50-64` | | Core.VirtualTags-008 | Medium | Resolved | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:81-115` | | Core.VirtualTags-012 | Medium | Resolved | Testing coverage | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags.Tests/` | +| Core.VirtualTags-014 | Medium | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:400` | | Driver.AbCip-004 | Medium | Resolved | Correctness & logic bugs | `AbCipDataType.cs:51-58`, `LibplctagTagRuntime.cs:47-49,53` | | Driver.AbCip-005 | Medium | Resolved | Correctness & logic bugs | `AbCipDriver.cs:124-141` | | Driver.AbCip-006 | Medium | Resolved | OtOpcUa conventions | `PlcTagHandle.cs:28-59`, `AbCipDriver.cs:806-807,832-833`, `LibplctagTagRuntime.cs:117` | @@ -288,6 +291,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Analyzers-004 | Low | Resolved | Performance & resource management | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:95-112` | | Analyzers-005 | Low | Resolved | Design-document adherence | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:33-43` | | Analyzers-007 | Low | Resolved | Documentation & comments | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:21-26` | +| Analyzers-008 | Low | Resolved | Testing coverage | `tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs` | | Client.CLI-002 | Low | Resolved | Correctness & logic bugs | `Commands/SubscribeCommand.cs:129-137` | | Client.CLI-003 | Low | Resolved | Correctness & logic bugs | `Commands/BrowseCommand.cs:29-30`, `Commands/SubscribeCommand.cs:20-27`, `Commands/AlarmsCommand.cs:28-29`, `Commands/HistoryReadCommand.cs:42-43` | | Client.CLI-004 | Low | Resolved | OtOpcUa conventions | `Commands/SubscribeCommand.cs:13-37` | @@ -339,6 +343,7 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | 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.AlarmHistorian-013 | Low | Resolved | Concurrency & thread safety | `src/Core/ZB.MOM.WW.OtOpcUa.Core.AlarmHistorian/SqliteStoreAndForwardSink.cs:79` | | Core.ScriptedAlarms-003 | Low | Resolved | Documentation & comments | `ScriptedAlarmEngine.cs:343`, `docs/ScriptedAlarms.md:107` | | Core.ScriptedAlarms-006 | Low | Resolved | Concurrency & thread safety | `ScriptedAlarmEngine.cs:232`, `ScriptedAlarmEngine.cs:369` | | Core.ScriptedAlarms-008 | Low | Resolved | Performance & resource management | `Part9StateMachine.cs:261-268` | @@ -346,6 +351,8 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.ScriptedAlarms-010 | Low | Resolved | Design-document adherence | `ScriptedAlarmEngine.cs:325-336`, `AlarmPredicateContext.cs:33-40`, `MessageTemplate.cs:47` | | Core.ScriptedAlarms-011 | Low | Resolved | Code organization & conventions | `Part9StateMachine.cs:275` | | Core.ScriptedAlarms-013 | Low | Resolved | Documentation & comments | `ScriptedAlarmEngine.cs:66-81` | +| Core.ScriptedAlarms-014 | Low | Resolved | Code organization & conventions | `ScriptedAlarmEngine.cs:539-540` (pre-fix) | +| Core.ScriptedAlarms-015 | Low | Resolved | Error handling & resilience | `ScriptedAlarmEngine.cs:284-298` (pre-fix) / `ScriptedAlarmEngine.cs:184-198` (post-fix) | | Core.Scripting-005 | Low | Resolved | Correctness & logic bugs | `DependencyExtractor.cs:97` | | Core.Scripting-006 | Low | Resolved | Concurrency & thread safety | `CompiledScriptCache.cs:55` | | Core.Scripting-008 | Low | Resolved | Performance & resource management | `CompiledScriptCache.cs:34`, `ScriptEvaluator.cs:34` | @@ -365,6 +372,8 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Core.VirtualTags-010 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/ITagUpstreamSource.cs:18`, `VirtualTagContext.cs:30`, `VirtualTagDefinition.cs:28` | | Core.VirtualTags-011 | Low | Resolved | Code organization & conventions | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:404-409` | | Core.VirtualTags-013 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/DependencyGraph.cs:266-270` | +| Core.VirtualTags-015 | Low | Resolved | Correctness & logic bugs | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/TimerTriggerScheduler.cs:55-74` | +| Core.VirtualTags-016 | Low | Resolved | Documentation & comments | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:472-475` | | Driver.AbCip-007 | Low | Resolved | OtOpcUa conventions | `AbCipDriver.cs` (whole file), `AbCipAlarmProjection.cs`, `LibplctagTagRuntime.cs` | | Driver.AbCip-011 | Low | Resolved | Error handling & resilience | `AbCipDriver.cs:144-152`, `AbCipDriverOptions.cs:131-143` | | Driver.AbCip-012 | Low | Resolved | Performance & resource management | `LibplctagTemplateReader.cs:15-35`, `AbCipDriver.cs:88-92` | @@ -388,6 +397,8 @@ Findings with status `Resolved`, `Won't Fix`, or `Deferred`. | Driver.Cli.Common-004 | Low | Resolved | Error handling & resilience | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:68-70` | | Driver.Cli.Common-006 | Low | Resolved | Documentation & comments | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:71`, `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:9` | | Driver.Cli.Common-008 | Low | Resolved | Testing coverage | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:50-64` | +| Driver.Cli.Common-009 | Low | Resolved | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:159-161` | +| Driver.Cli.Common-010 | Low | Resolved | Testing coverage | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:167-172` / `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs` | | Driver.FOCAS-007 | Low | Resolved | Error handling & resilience | `FocasDriver.cs:140-148`, `FocasDriver.cs:478-484`, `FocasDriver.cs:529-533`, `FocasAlarmProjection.cs:61-63` | | Driver.FOCAS-008 | Low | Resolved | Performance & resource management | `FocasDriver.cs:201`, `FocasDriver.cs:253` | | Driver.FOCAS-009 | Low | Resolved | Design-document adherence | `FocasDriverOptions.cs:110-115`, `FocasDriver.cs:468-486`, `FocasDriverFactoryExtensions.cs:75-80` |