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	12

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	Open
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

Unresolved.

Commons-002 — DynamicJsonElement retains a JsonElement whose JsonDocument lifetime it does not own

Severity	Medium
Category	Performance & resource management
Status	Open
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

Unresolved.

Commons-003 — ScriptParameters.GetNullable silently swallows conversion failures

Severity	Medium
Category	Error handling & resilience
Status	Open
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

Unresolved.

Commons-004 — ManagementCommandRegistry name mapping is asymmetric and namespace-scoped

Severity	Medium
Category	Code organization & conventions
Status	Open
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

Unresolved.

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.