From ad4744a2955410d8ac87e873b741c7c728ad7132 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 24 Jun 2026 09:40:47 -0400 Subject: [PATCH] docs(code-review): resolve the 8 re-review findings (fixed in 9ab1c002) Flip DataConnectionLayer-029, InboundAPI-031, SiteRuntime-032/033, StoreAndForward-028, AuditLog-017, CentralUI-037, ScriptAnalysis-009 from Open to Resolved with the fixing commit + a one-line resolution each; regen README (0 pending / 576 total across 25 modules). --- code-reviews/AuditLog/findings.md | 6 ++-- code-reviews/CentralUI/findings.md | 6 ++-- code-reviews/DataConnectionLayer/findings.md | 6 ++-- code-reviews/InboundAPI/findings.md | 6 ++-- code-reviews/README.md | 38 ++++++++------------ code-reviews/ScriptAnalysis/findings.md | 6 ++-- code-reviews/SiteRuntime/findings.md | 10 +++--- code-reviews/StoreAndForward/findings.md | 6 ++-- 8 files changed, 37 insertions(+), 47 deletions(-) diff --git a/code-reviews/AuditLog/findings.md b/code-reviews/AuditLog/findings.md index e3c94b13..ef6d279b 100644 --- a/code-reviews/AuditLog/findings.md +++ b/code-reviews/AuditLog/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -905,7 +905,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Low | | Category | Testing coverage | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.AuditLog/Central/AuditLogIngestActor.cs:149` | **Description** @@ -918,4 +918,4 @@ Add a TestKit test using a service provider whose IAuditLogRepository resolution **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): added `Receive_WhenRepositoryResolutionThrows_ActorSurvives_RepliesEmpty_CountsFailure` to `AuditLogIngestActorTests` — it drives the production-ctor actor with a service provider that has no `IAuditLogRepository` registered (so the per-message scope resolution throws), asserting the actor survives, replies with an empty accepted list, and increments the failure counter on each of two successive batches. The AuditLog-014 guard path is now pinned. diff --git a/code-reviews/CentralUI/findings.md b/code-reviews/CentralUI/findings.md index afe96771..2f98eba9 100644 --- a/code-reviews/CentralUI/findings.md +++ b/code-reviews/CentralUI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -1715,7 +1715,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/ScriptAnalysisService.cs:163` | **Description** @@ -1728,4 +1728,4 @@ Extend the line-163 sentence to note that the inbound sandbox surface also throw **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): extended the `RunInSandboxAsync` XML summary to note that the inbound sandbox surface also throws on every `Database` call (`QuerySingleAsync`/`QueryAsync`/`ExecuteAsync`) because a Test Run has no configured central DB connection, mirroring the existing `Route` wording. diff --git a/code-reviews/DataConnectionLayer/findings.md b/code-reviews/DataConnectionLayer/findings.md index 7c7647bc..e270e881 100644 --- a/code-reviews/DataConnectionLayer/findings.md +++ b/code-reviews/DataConnectionLayer/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -1534,7 +1534,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/Actors/DataConnectionActor.cs:1823` | **Description** @@ -1547,4 +1547,4 @@ In HandleAlarmSubscribeCompleted, before storing the subscription id, guard agai **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): `HandleAlarmSubscribeCompleted` now guards the `_alarmSubscriptionIds` store — when a feed is already present for the source, the redundant just-created subscription is released via `UnsubscribeAlarmsAsync` instead of overwriting the stored id (mirroring the tag-path re-check). Regression test `DCL029_ResubscribeDuringOrphanedInFlightSubscribe_ReleasesDuplicateFeed_NoLeak` added. diff --git a/code-reviews/InboundAPI/findings.md b/code-reviews/InboundAPI/findings.md index 1750f284..dd4d5a52 100644 --- a/code-reviews/InboundAPI/findings.md +++ b/code-reviews/InboundAPI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -1663,7 +1663,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs:266` | **Description** @@ -1676,4 +1676,4 @@ Align the local backstop with the round-trip slack the CommunicationService Ask **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): removed `WaitForAttribute`'s local `timeout + 5s` backstop CTS; the wait now links only the client-abort and explicit caller tokens and relies on `CommunicationService.RouteToWaitForAttributeAsync`'s `timeout + IntegrationTimeout` Ask bound as the authoritative round-trip backstop, so a slow-but-valid timed-out response returns `false` rather than being cancelled into a 500. Regression test `WaitForAttribute_SlowTimedOutResponse_NotPreemptedByLocalBackstop` added. diff --git a/code-reviews/README.md b/code-reviews/README.md index 12bed095..832decc0 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -41,37 +41,37 @@ module file and counted in **Total**. |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 4 | -| Low | 4 | -| **Total** | **8** | +| Medium | 0 | +| Low | 0 | +| **Total** | **0** | ## Module Status | Module | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |--------|---------------|--------|----------------|------|-------| -| [AuditLog](AuditLog/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/1 | 1 | 17 | +| [AuditLog](AuditLog/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 17 | | [CLI](CLI/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 24 | -| [CentralUI](CentralUI/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/1 | 1 | 37 | +| [CentralUI](CentralUI/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 37 | | [ClusterInfrastructure](ClusterInfrastructure/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 15 | | [Commons](Commons/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 26 | | [Communication](Communication/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 24 | | [ConfigurationDatabase](ConfigurationDatabase/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 27 | -| [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/1/0 | 1 | 27 | +| [DataConnectionLayer](DataConnectionLayer/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 27 | | [DeploymentManager](DeploymentManager/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 27 | | [ExternalSystemGateway](ExternalSystemGateway/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 26 | | [HealthMonitoring](HealthMonitoring/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 25 | | [Host](Host/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 26 | -| [InboundAPI](InboundAPI/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/1/0 | 1 | 31 | +| [InboundAPI](InboundAPI/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 31 | | [KpiHistory](KpiHistory/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 6 | | [ManagementService](ManagementService/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 26 | | [NotificationOutbox](NotificationOutbox/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 13 | | [NotificationService](NotificationService/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 28 | -| [ScriptAnalysis](ScriptAnalysis/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/1 | 1 | 9 | +| [ScriptAnalysis](ScriptAnalysis/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 9 | | [Security](Security/findings.md) | 2026-06-20 | `4307c381` | 0/0/0/0 | 0 | 25 | | [SiteCallAudit](SiteCallAudit/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 9 | | [SiteEventLogging](SiteEventLogging/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 27 | -| [SiteRuntime](SiteRuntime/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/1/1 | 2 | 33 | -| [StoreAndForward](StoreAndForward/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/1/0 | 1 | 28 | +| [SiteRuntime](SiteRuntime/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 33 | +| [StoreAndForward](StoreAndForward/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 28 | | [TemplateEngine](TemplateEngine/findings.md) | 2026-06-24 | `1f9de8a2` | 0/0/0/0 | 0 | 25 | | [Transport](Transport/findings.md) | 2026-06-19 | `d6ead8ae` | 0/0/0/0 | 0 | 15 | @@ -90,20 +90,10 @@ _None open._ _None open._ -### Medium (4) +### Medium (0) -| ID | Module | Title | -|----|--------|-------| -| DataConnectionLayer-029 | [DataConnectionLayer](DataConnectionLayer/findings.md) | Resubscribe during orphaned in-flight subscribe leaks an alarm feed | -| InboundAPI-031 | [InboundAPI](InboundAPI/findings.md) | Wait grace backstop tighter than the layer's round-trip slack | -| SiteRuntime-032 | [SiteRuntime](SiteRuntime/findings.md) | Deleting a disabled instance leaks the deployed count | -| StoreAndForward-028 | [StoreAndForward](StoreAndForward/findings.md) | StopAsync resets register-guard but not _bufferedCount, double-counting gauge on restart | +_None open._ -### Low (4) +### Low (0) -| ID | Module | Title | -|----|--------|-------| -| AuditLog-017 | [AuditLog](AuditLog/findings.md) | AuditLog-014 guard path not exercised by a test | -| CentralUI-037 | [CentralUI](CentralUI/findings.md) | RunInSandboxAsync doc comment omits new Database-throws behavior | -| ScriptAnalysis-009 | [ScriptAnalysis](ScriptAnalysis/findings.md) | Inline Pass 1 comment no longer reflects parameterized base references | -| SiteRuntime-033 | [SiteRuntime](SiteRuntime/findings.md) | Native-alarm doc stale re: per-condition eviction and cap return-to-normal | +_None open._ diff --git a/code-reviews/ScriptAnalysis/findings.md b/code-reviews/ScriptAnalysis/findings.md index bf29888f..d99b1b37 100644 --- a/code-reviews/ScriptAnalysis/findings.md +++ b/code-reviews/ScriptAnalysis/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -435,7 +435,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptTrustValidator.cs:106` | **Description** @@ -448,4 +448,4 @@ Reword the comment to note that Pass 1 resolves against the supplied baseReferen **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): reworded the Pass 1 comment to state it resolves against the supplied `baseReferences` — the full trusted-platform reference set on the public entry point, or the minimal anchor-enriched fallback (`BuildMinimalFallbackReferences()`) on the degraded/test path. diff --git a/code-reviews/SiteRuntime/findings.md b/code-reviews/SiteRuntime/findings.md index f8ae6da7..b5d04ac1 100644 --- a/code-reviews/SiteRuntime/findings.md +++ b/code-reviews/SiteRuntime/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 2 | +| Open findings | 0 | ## Summary @@ -1668,7 +1668,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs:660-690` | **Description** @@ -1683,7 +1683,7 @@ Treat 'has a persisted deployed config' as present, not just 'in one of the two **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): replaced the `_totalDeployedCount` int (gated on in-memory map presence) with an authoritative `_deployedInstanceNames` HashSet; the deployed/disabled health counts are derived from its size, so deleting a disabled instance (absent from both maps) now decrements correctly while a delete for a never-deployed instance is a no-op. Regression test `SR032_DeleteDisabledInstance_DecrementsDeployedCount` added. ### SiteRuntime-033 — Native-alarm doc stale re: per-condition eviction and cap return-to-normal @@ -1691,7 +1691,7 @@ _Unresolved._ |--|--| | Severity | Low | | Category | Documentation & comments | -| Status | Open | +| Status | Resolved | | Location | `docs/requirements/Component-SiteRuntime.md:272` | **Description** @@ -1704,4 +1704,4 @@ Update the Per-source cap bullet to note that an active evicted condition emits **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): updated `Component-SiteRuntime.md` — the per-source-cap bullet now documents the return-to-normal emit for an active evicted condition, and the reset-semantics bullet documents per-condition `_latestAlarmEvents` eviction via `NativeAlarmDropped` (snapshot-swap / retention drop / cap eviction), not only redeploy/undeploy. diff --git a/code-reviews/StoreAndForward/findings.md b/code-reviews/StoreAndForward/findings.md index 95bd9c98..19e35cfc 100644 --- a/code-reviews/StoreAndForward/findings.md +++ b/code-reviews/StoreAndForward/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-06-24 | | Reviewer | claude-agent | | Commit reviewed | `1f9de8a2` | -| Open findings | 1 (025 Resolved; 026, 027 Deferred; 5 Deferred total: 002, 011, 012, 026, 027; all prior Open findings resolved) | +| Open findings | 0 (025 Resolved; 026, 027 Deferred; 5 Deferred total: 002, 011, 012, 026, 027; all prior Open findings resolved) | ## Summary @@ -1765,7 +1765,7 @@ Focused re-review of the changes since the prior review — verifying the code-r |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ZB.MOM.WW.ScadaBridge.StoreAndForward/StoreAndForwardService.cs:459` | **Description** @@ -1778,4 +1778,4 @@ Reset the cached counter on stop alongside the guard, e.g. `Interlocked.Exchange **Resolution** -_Unresolved._ +Resolved in commit `9ab1c002` (2026-06-24): `StopAsync` now resets `_bufferedCount` to 0 alongside the registration guard, so a same-instance Stop->Start re-seeds the depth gauge from the durable Pending count on a clean base instead of doubling to ~2N. Regression test `StartAsync_AfterStop_ReseedsFromCleanBase_NoDoubleCount` added.