fix(commons): resolve Commons-001..004 — stale-fire race, JsonDocument lifetime, GetNullable strictness, registry symmetry

code-reviews/Commons/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 12 |
+| Open findings | 8 |
 
 ## Summary
 
@@ -55,7 +55,7 @@ wire command.
 |--|--|
 | Severity | Medium |
 | Category | Concurrency & thread safety |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Commons/Types/StaleTagMonitor.cs:42-46`, `:62-67` |
 
 **Description**
@@ -81,7 +81,14 @@ only then reschedule the timer.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending) — confirmed the race against the source. Replaced
+the `volatile bool` guard with a lock-protected monotonic generation token: `Start`,
+`OnValueReceived` and `Stop` each bump the generation under a gate, and the timer
+callback only raises `Stale` if its scheduled generation still matches. `OnValueReceived`
+now recreates the timer (rather than `Change`-ing it) so the rescheduled callback carries
+the new token. A superseded or stopped period can no longer emit a spurious staleness
+signal. Regression tests added in `StaleTagMonitorRaceTests` (deterministic via an
+internal `CallbackEnteredHook` test seam).
 
 ### Commons-002 — `DynamicJsonElement` retains a `JsonElement` whose `JsonDocument` lifetime it does not own
 
@@ -89,7 +96,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:10-17` |
 
 **Description**
@@ -113,7 +120,13 @@ regardless.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending) — confirmed the hazard: `ExternalCallResult.Response`
+constructs the wrapper from `JsonDocument.Parse(...).RootElement` with no reference kept
+to the document, so deferred script-time access could fault. Fixed at the root by cloning
+the element with `JsonElement.Clone()` in the `DynamicJsonElement` constructor, detaching
+it from the owning document; the public constructor signature is unchanged. Added a
+remarks block documenting the lifetime contract. Regression tests added in
+`DynamicJsonElementTests` (access after the source document is disposed / GC-collected).
 
 ### Commons-003 — `ScriptParameters.GetNullable` silently swallows conversion failures
 
@@ -121,7 +134,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Commons/Types/ScriptParameters.cs:72-86` |
 
 **Description**
@@ -146,7 +159,15 @@ element handling. If the swallowing must stay for compatibility, at minimum surf
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending) — confirmed the silent-swallow path against the
+source. Removed the `catch (ScriptParameterException)` block in `GetNullable<T>`: an
+absent or explicitly-null parameter still returns `null`, but a parameter that is
+*present but holds an unconvertible value* now throws `ScriptParameterException` with a
+descriptive message, consistent with `Get<T>()` and the array/list element paths. The
+`Get<T>` XML doc was corrected accordingly. This is a deliberate behavioral change toward
+correctness — the previous behavior masked caller/script bugs; the type-level public
+contract is unchanged. Regression tests added in `ScriptParametersTests`
+(`Get_NullableInt_PresentButUnparsable_Throws` and siblings).
 
 ### Commons-004 — `ManagementCommandRegistry` name mapping is asymmetric and namespace-scoped
 
@@ -154,7 +175,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Code organization & conventions |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs:14-35` |
 
 **Description**
@@ -187,7 +208,20 @@ scope, and reconsider whether the `Mgmt*` prefixed duplicates are still needed.
 
 **Resolution**
 
-_Unresolved._
+Resolved 2026-05-16 (commit pending) — confirmed the asymmetry: `GetCommandName` stripped
+`Command` from any type while `BuildRegistry` only registered the `Messages.Management`
+namespace. In practice no defect was observed because every command type the CLI and
+ManagementService actually use is in `Messages.Management` (a round-trip test over all
+registered commands confirms no name collision). Closed the asymmetry by making
+`GetCommandName` registry-bound: it now looks up a reverse `Type→name` frozen dictionary
+built from the same registry and throws `ArgumentException` for any unregistered type, so
+`Resolve(GetCommandName(t)) == t` holds for every type it accepts. Added an XML remarks
+block documenting the registry scope and the symmetry guarantee. The `Mgmt*` prefixed
+records were left in place — they are the genuine Management-namespace command types the
+CLI constructs and renaming them would change wire command names (out of scope for a
+behavior-preserving fix; noted for a future cleanup). CLI, ManagementService, and
+SiteRuntime all build clean against the change. Regression tests added in
+`ManagementCommandRegistryTests`.
 
 ### Commons-005 — `OpcUaEndpointConfigSerializer.Deserialize` discards malformed legacy input and over-reports `IsLegacy`