scadalink-design/code-reviews/Commons/findings.md

# Code Review — Commons

| Field | Value |
|-------|-------|
| Module | `src/ScadaLink.Commons` |
| Design doc | `docs/requirements/Component-Commons.md` |
| Status | Reviewed |
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 8 |

## Summary

Commons is in good overall health. It is a well-organized, dependency-light library:
the architectural-constraint tests enforce the no-Akka/no-EF/no-ASP.NET rule, the
POCO-entity and message-as-record conventions, and the UTC timestamp rule. The folder
and namespace hierarchy closely matches REQ-COM-5b. No Critical issues were found.

The findings cluster around three themes. First, a handful of files quietly stretch
the REQ-COM-6 "no business logic" boundary — `StaleTagMonitor`, `OpcUaEndpointConfigSerializer`,
`OpcUaEndpointConfigValidator`, `ScriptParameters`, `ValueFormatter`, `DynamicJsonElement`
and `ScriptArgs` all carry non-trivial behavior, and a couple have real correctness or
concurrency defects (the `StaleTagMonitor` stale-fire race, the `DynamicJsonElement`
`JsonDocument`-lifetime hazard, the silent conversion-failure swallowing in
`ScriptParameters.GetNullable`). Second, the `ManagementCommandRegistry` name mapping is
asymmetric and namespace-scoped in a way that does not match the broader set of
`*Command` records elsewhere in `Messages/`. Third, several behavior-bearing types
(`ValueFormatter`, `DynamicJsonElement`, `ScriptArgs`, `ManagementCommandRegistry`,
`Result<T>`, the OPC UA serializer round-trip) have no unit tests despite containing the
kind of edge-case logic that warrants them. Entity and message contracts otherwise look
clean and additive-evolution-friendly, with the exception of one `ValueTuple` use in a
wire command.

## Checklist coverage

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ✓ | `DynamicJsonElement.TryConvert` returns success for non-convertible types; `Result<T>` allows null error; legacy-config fallback loses data. |
| 2 | Akka.NET conventions | ✓ | Commons has no actors (correct). Message contracts are records and immutable. One wire message uses `ValueTuple` (Commons-008). Correlation IDs present on request/response messages. |
| 3 | Concurrency & thread safety | ✓ | `StaleTagMonitor` has a check-then-act race between the timer callback and `OnValueReceived` (Commons-001). |
| 4 | Error handling & resilience | ✓ | `ScriptParameters.GetNullable` silently swallows conversion failures (Commons-003); OPC UA legacy deserialize discards malformed input (Commons-005). |
| 5 | Security | ✓ | No auth logic here. `SmtpConfiguration.Credentials` / OPC UA passwords are plain-string fields (storage/encryption is a consumer concern) — noted, not a finding. No script-trust violations: Commons defines no forbidden-API surface. |
| 6 | Performance & resource management | ✓ | `StaleTagMonitor` disposes its `Timer` correctly. `DynamicJsonElement` references a `JsonElement` whose backing document lifetime is not owned (Commons-002). |
| 7 | Design-document adherence | ✓ | Several behavior-bearing helper/validator/serializer classes push against REQ-COM-6 "no business logic" (Commons-007). Folder layout matches REQ-COM-5b. |
| 8 | Code organization & conventions | ✓ | `ManagementCommandRegistry` naming is asymmetric/namespace-scoped (Commons-004). `DeployedConfigSnapshot`, `InstanceAlarmOverride`, `TemplateFolder`, `ISiteRepository`, several service interfaces and `Messages/Management` exist but are not listed in Component-Commons.md (Commons-009). |
| 9 | Testing coverage | ✓ | `ValueFormatter`, `DynamicJsonElement`, `ScriptArgs`, `ManagementCommandRegistry`, `Result<T>`, `ConfigurationDiff`, `AlarmContext`, and the OPC UA serializer round-trip have no tests (Commons-010). |
| 10 | Documentation & comments | ✓ | `OpcUaEndpointConfigSerializer.Deserialize` XML doc does not mention the silent data-loss path (Commons-005). `Component-Commons.md` is stale relative to the actual file set (Commons-009). `ValueFormatter` uses current-culture formatting without documenting it (Commons-012). |

## Findings

### Commons-001 — `StaleTagMonitor` stale-fire race between timer and `OnValueReceived`

| | |
|--|--|
| Severity | Medium |
| Category | Concurrency & thread safety |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/StaleTagMonitor.cs:42-46`, `:62-67` |

**Description**

`OnValueReceived` sets `_staleFired = false` then calls `_timer.Change(...)`, while the
timer callback `OnTimerElapsed` reads `_staleFired`, sets it to `true`, and invokes the
`Stale` event. `_staleFired` is `volatile`, which guarantees visibility but not
atomicity of the check-then-set. The two methods run on different threads (a value-
arrival thread and a `ThreadPool` timer thread). If the timer callback has already
passed the `if (_staleFired) return;` check when `OnValueReceived` runs, `Stale` fires
even though a fresh value just arrived — a spurious staleness signal. There is also a
window where `OnValueReceived` resets `_staleFired` and reschedules the timer while a
callback for the previous period is mid-flight, so `Stale` can fire once per period as
documented but at the wrong moment. For a heartbeat monitor feeding connection-health
decisions, a false stale signal can trigger an unnecessary reconnect.

**Recommendation**

Guard the state transition with a lock, or replace the `_staleFired` bool with an
`Interlocked.CompareExchange` on an `int` so only one of "fire" / "reset" wins. The
callback should atomically test-and-set; `OnValueReceived` should atomically reset and
only then reschedule the timer.

**Resolution**

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

| | |
|--|--|
| Severity | Medium |
| Category | Performance & resource management |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:10-17` |

**Description**

`DynamicJsonElement` stores a `JsonElement` and exposes it for deferred, dynamic access
from scripts. A `JsonElement` is only valid while the `JsonDocument` that produced it has
not been disposed; accessing a `JsonElement` after its document is disposed throws
`ObjectDisposedException`. Nothing in `DynamicJsonElement` keeps the document alive or
documents that the caller must. Because the wrapper is explicitly designed for
"convenient property access in scripts" — i.e. access at an arbitrary later time — a
caller that wraps an element from a `using var doc = JsonDocument.Parse(...)` block (the
exact pattern used in `OpcUaEndpointConfigSerializer`) will hand scripts a wrapper that
faults on first member access.

**Recommendation**

Either clone the element on construction with `JsonElement.Clone()` (which detaches it
from the document and makes it safe to retain), or hold a reference to the owning
`JsonDocument` and implement `IDisposable`. Document the lifetime contract on the type
regardless.

**Resolution**

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

| | |
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/ScriptParameters.cs:72-86` |

**Description**

`GetNullable<T>` catches `ScriptParameterException` from `ConvertScalar` and returns
`default!` (null) "on conversion failure for nullable". This conflates two distinct
cases: a parameter that is genuinely absent/null, and a parameter that is *present but
holds an unconvertible value* (e.g. `Get<int?>("count")` when `count` is the string
`"banana"`). The latter is almost always a script or caller bug, and silently mapping it
to `null` hides it — the script then proceeds with a null it interprets as "not
supplied". The non-nullable `Get<T>` and the array/list paths correctly throw with a
descriptive message for the same bad input, so the behavior is also inconsistent across
the API surface. The XML doc states "returns null if missing, null, or unconvertible",
so the behavior is intentional, but it remains a footgun.

**Recommendation**

Distinguish "absent/null" from "present but unconvertible": return null only for the
former and throw `ScriptParameterException` for the latter, mirroring the array/list
element handling. If the swallowing must stay for compatibility, at minimum surface it
(e.g. an out-of-band warning) rather than failing silently.

**Resolution**

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

| | |
|--|--|
| Severity | Medium |
| Category | Code organization & conventions |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Messages/Management/ManagementCommandRegistry.cs:14-35` |

**Description**

`BuildRegistry` registers only types in the exact `ScadaLink.Commons.Messages.Management`
namespace whose names end in `Command`. `GetCommandName(Type)`, however, strips a
`Command` suffix from *any* type passed to it. The two halves disagree:

- `GetCommandName` will happily compute a command name for `*Command` records that live
  in other `Messages/` sub-namespaces (`DeployInstanceCommand` in `Messages.Deployment`,
  `DisableInstanceCommand` in `Messages.Lifecycle`, `SetStaticAttributeCommand` in
  `Messages.Instance`, `DeployArtifactsCommand` in `Messages.Artifacts`, etc.), yet
  `Resolve` will return `null` for every one of those names because they were never
  registered.
- Because of this gap the Management namespace carries deliberately renamed duplicates
  (`MgmtDeployInstanceCommand`, `MgmtEnableInstanceCommand`, `MgmtDisableInstanceCommand`,
  `MgmtDeleteInstanceCommand` in `InstanceCommands.cs`) whose `Mgmt` prefix exists only
  to dodge a collision the registry's namespace filter already prevents — a confusing,
  undocumented coupling.

A round-trip `Resolve(GetCommandName(t))` is therefore not guaranteed to return `t`,
which is the implicit contract of a name registry.

**Recommendation**

Make the two methods symmetric: either scan all of `Messages/` (and detect/throw on
duplicate stripped names, since `ToFrozenDictionary` will throw on a collision) or
restrict `GetCommandName` to types the registry actually contains. Document the chosen
scope, and reconsider whether the `Mgmt*` prefixed duplicates are still needed.

**Resolution**

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`

| | |
|--|--|
| Severity | Low |
| Category | Error handling & resilience |
| Status | Open |
| Location | `src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs:25-51` |

**Description**

When the typed-deserialize path fails or the JSON lacks `endpointUrl`, `Deserialize`
falls through to `LoadLegacy`. If `LoadLegacy` itself throws `JsonException` (genuinely
malformed JSON), the method returns `(new OpcUaEndpointConfig(), IsLegacy: true)` — a
default, empty config with the legacy flag set. The original stored string is silently
discarded, and the caller is told it is a recoverable "legacy" row when in fact the data
was unparseable. A form built on the documented `IsLegacy` contract ("prompt the user to
re-save") will present an empty config as if it were the user's saved configuration,
inviting them to overwrite real (if malformed) data with blanks. The XML doc only
describes the happy legacy path and does not mention this data-loss branch.

**Recommendation**

Distinguish "parsed as legacy" from "could not parse at all" — e.g. return a third state
or throw for genuinely malformed input so the caller can surface an error instead of an
empty form. Update the XML doc to describe the failure branch.

**Resolution**

_Unresolved._

### Commons-006 — `DynamicJsonElement.TryConvert` reports success for unconvertible target types

| | |
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Location | `src/ScadaLink.Commons/Types/DynamicJsonElement.cs:47-51`, `:66-76` |

**Description**

`TryConvert` does `result = ConvertTo(binder.Type); return result != null || binder.Type == typeof(object);`.
`ConvertTo` returns `null` for any type/kind pair it does not handle (e.g. requesting
`int` from a JSON string, or `DateTime` from anything). For a non-`object` target this
yields `result == null` and `return false`, which is correct. But the `|| binder.Type == typeof(object)`
clause makes `(object)dynamicElement` succeed with a `null` result even when the wrapped
element is, say, a JSON object or a non-null string — the cast silently produces `null`
instead of the element or its value. Any script doing `object o = jsonThing;` gets `null`
for a present value. The conversion of a present, non-null JSON value should never yield
`null`.

**Recommendation**

For the `object` target, return the element itself (or `Wrap(_element)`) rather than
`null`. Only return `null` when the wrapped element is genuinely `JsonValueKind.Null`.

**Resolution**

_Unresolved._

### Commons-007 — Several Commons types carry non-trivial logic, stretching REQ-COM-6

| | |
|--|--|
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Location | `src/ScadaLink.Commons/Types/ScriptParameters.cs`, `src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs`, `src/ScadaLink.Commons/Validators/OpcUaEndpointConfigValidator.cs`, `src/ScadaLink.Commons/Types/StaleTagMonitor.cs`, `src/ScadaLink.Commons/Types/ScriptArgs.cs` |

**Description**

REQ-COM-6 states Commons "must contain only data structures, interfaces, enums, and
constants" and "must not contain any business logic", with method bodies "limited to
trivial data-access logic". Several files exceed that: `ScriptParameters` performs typed
conversion with reflection and JSON-element unwrapping; `OpcUaEndpointConfigSerializer`
implements a multi-shape (typed + legacy flat-dict) serialization strategy;
`OpcUaEndpointConfigValidator` encodes OPC UA domain rules (e.g. `LifetimeCount` ≥ 3×
`KeepAliveCount`); `StaleTagMonitor` runs a `Timer` and raises events; `ScriptArgs`
reflects over arbitrary objects. The `ArchitecturalConstraintTests` "no service/actor"
heuristic only counts public methods (> 3) and so does not catch these. This is design
drift, not a defect — but it should be a deliberate decision: either move these helpers
into the components that own the behavior (Data Connection Layer, Site Runtime,
Template Engine) or amend Component-Commons.md to explicitly permit "pure stateless
helpers/validators".

**Recommendation**

Decide and document the policy. If these are intentionally allowed in Commons, add a
sentence to REQ-COM-6 carving out pure validators/serializers/parsers; otherwise relocate
them. Tighten the architectural test if the rule is meant to be enforced.

**Resolution**

_Unresolved._

### Commons-008 — `SetConnectionBindingsCommand` uses `ValueTuple` in a wire message contract

| | |
|--|--|
| Severity | Low |
| Category | Akka.NET conventions |
| Status | Open |
| Location | `src/ScadaLink.Commons/Messages/Management/InstanceCommands.cs:10` |

**Description**

`SetConnectionBindingsCommand` declares
`IReadOnlyList<(string AttributeName, int DataConnectionId)> Bindings`. The tuple element
names are compile-time-only; `System.Text.Json` serializes a `ValueTuple` as `Item1` /
`Item2`, and the message is positional with no room for additive evolution (you cannot
add a third field without changing the tuple type, which REQ-COM-5a forbids). Every other
message in `Messages/` uses named records. A management command travels over the
ClusterClient boundary and is exactly the kind of contract REQ-COM-5a's additive-only
rule targets.

**Recommendation**

Replace the tuple with a small named record, e.g.
`record ConnectionBinding(string AttributeName, int DataConnectionId)`, and use
`IReadOnlyList<ConnectionBinding>`.

**Resolution**

_Unresolved._

### Commons-009 — `Component-Commons.md` is stale relative to the actual file set

| | |
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Location | `docs/requirements/Component-Commons.md:61-198` |

**Description**

The design doc's entity list, repository list, and folder tree no longer match the code:

- Entities present but undocumented: `DeployedConfigSnapshot`, `InstanceAlarmOverride`,
  `TemplateFolder`.
- Repository interface present but undocumented: `ISiteRepository` (the doc lists seven
  repositories under REQ-COM-4; the code has eight).
- Service interfaces present but undocumented: `IDatabaseGateway`,
  `IExternalSystemClient`, `IInstanceLocator`, `INotificationDeliveryService` — REQ-COM-4a
  documents only `IAuditService`.
- Whole namespaces absent from the REQ-COM-5b folder tree: `Messages/Management`,
  `Messages/DataConnection`, `Messages/Integration`, `Messages/Instance`,
  `Messages/RemoteQuery`, plus `Types/DataConnections`, `Types/Scripts`, `Serialization/`,
  and `Validators/`.

CLAUDE.md's editing rules require the design docs to stay in sync with the code; the doc
is now a partial map.

**Recommendation**

Refresh Component-Commons.md to enumerate the current entities, repository and service
interfaces, and the actual `Types/`, `Messages/`, `Serialization/`, and `Validators/`
folders.

**Resolution**

_Unresolved._

### Commons-010 — Behavior-bearing Commons types have no unit tests

| | |
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Location | `tests/ScadaLink.Commons.Tests/` |

**Description**

`ScadaLink.Commons.Tests` covers `Result`, `RetryPolicy`, `ScriptParameters`,
`StaleTagMonitor`, the OPC UA validator, enums, message conventions, compatibility, and
entity conventions. It does not cover several types that contain exactly the kind of
edge-case logic that warrants tests:

- `ValueFormatter` — scalar vs collection vs null formatting.
- `DynamicJsonElement` — member/index access, conversions, the issues in Commons-002 and
  Commons-006 would have been caught by tests.
- `ScriptArgs.Normalize` — dictionary/anonymous-object/primitive-rejection paths.
- `ManagementCommandRegistry` — `Resolve` / `GetCommandName` round-trip (would have
  surfaced Commons-004).
- `Result<T>` — `Match`, failure/success accessors, error-on-misuse.
- `OpcUaEndpointConfigSerializer` typed↔flat round-trip and legacy fallback.
- `ConfigurationDiff` / `AlarmContext` / `ScriptScope` — minor, but `HasChanges` /
  `HasParent` logic is untested.

**Recommendation**

Add focused unit tests for the helper/utility types above, prioritizing
`DynamicJsonElement`, `ScriptArgs`, `ManagementCommandRegistry`, and the OPC UA serializer
round-trip.

**Resolution**

_Unresolved._

### Commons-011 — `Result<T>.Failure` accepts a null error string

| | |
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Location | `src/ScadaLink.Commons/Types/Result.cs:15-20`, `:30-32`, `:36` |

**Description**

`Result<T>.Failure(string error)` and the private failure constructor do not validate
`error`. A caller passing `null` produces a failed `Result` whose `Error` getter returns
`null` via `_error!`, and whose `Match` calls `onFailure(_error!)` with `null`. `Result`
is the system-wide error-handling type ("consistent error handling across component
boundaries"); a failed result with no error message defeats its purpose and pushes a
`NullReferenceException` risk onto every consumer that logs or displays `Error`.

**Recommendation**

Throw `ArgumentNullException` (or `ArgumentException` for empty/whitespace) in
`Failure`/the failure constructor so a failed `Result` always carries a message.

**Resolution**

_Unresolved._

### Commons-012 — `ValueFormatter` uses current-culture formatting without documenting it

| | |
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Open |
| Location | `src/ScadaLink.Commons/Types/ValueFormatter.cs:20-27` |

**Description**

`FormatDisplayValue` formats `IFormattable` values (and collection elements) with the
parameterless `ToString()`, which uses the current thread culture. The XML doc calls this
"the value's natural string representation" without noting the culture dependency. The
same numeric or `DateTime` attribute value will render differently depending on the
server/UI locale — e.g. decimal separators, date order. CLAUDE.md mandates UTC for
timestamps and notes local-time conversion is "a UI display concern only"; if
`ValueFormatter` is used outside a UI rendering context (e.g. logging, event-log entries,
diff display) the culture-dependent output is inconsistent and a latent bug.

**Recommendation**

Decide whether `ValueFormatter` is a UI-only helper. If it can be used outside the UI,
format with `CultureInfo.InvariantCulture` (using the `IFormattable.ToString(null, IFormatProvider)`
overload). Either way, document the culture behavior on the method.

**Resolution**

_Unresolved._