38 KiB
Code Review — Commons
| Field | Value |
|---|---|
| Module | src/ScadaLink.Commons |
| Design doc | docs/requirements/Component-Commons.md |
| Status | Reviewed |
| Last reviewed | 2026-05-17 |
| Reviewer | claude-agent |
| Commit reviewed | 39d737e |
| Open findings | 0 |
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.
Re-review 2026-05-17 (commit 39d737e). All twelve prior findings (Commons-001
through Commons-012) are confirmed Resolved — the fixes are sound, well-targeted, and
backed by focused regression tests (StaleTagMonitorRaceTests, DynamicJsonElementTests,
ScriptParametersTests, ManagementCommandRegistryTests, OpcUaEndpointConfigSerializerTests,
ResultTests, ValueFormatterTests, ConnectionBindingSerializationTests,
FlatteningAndScriptScopeTests). The new files introduced since 9c60592
(TemplateAlarm lock/inherit fields, IExternalSystemRepository name-keyed lookups,
DeploymentStateQueryRequest/Response, ParameterDefinition) follow the established
POCO / record / additive-evolution conventions and carry round-trip compatibility tests.
Two new Low-severity findings were recorded this pass: a DynamicJsonElement array
indexer that rejects long indices (Commons-013) and an OpcUaEndpointConfigSerializer
legacy-fallback path that can mislabel a corrupt new-shape row as Legacy (Commons-014).
No Critical, High, or Medium issues were found.
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. Re-review: DynamicJsonElement.TryGetIndex rejects non-int indices (Commons-013). |
| 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). Re-review: corrupt typed OPC UA rows can fall through to the legacy path and be mislabelled Legacy (Commons-014). |
| 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:
GetCommandNamewill happily compute a command name for*Commandrecords that live in otherMessages/sub-namespaces (DeployInstanceCommandinMessages.Deployment,DisableInstanceCommandinMessages.Lifecycle,SetStaticAttributeCommandinMessages.Instance,DeployArtifactsCommandinMessages.Artifacts, etc.), yetResolvewill returnnullfor every one of those names because they were never registered.- Because of this gap the Management namespace carries deliberately renamed duplicates
(
MgmtDeployInstanceCommand,MgmtEnableInstanceCommand,MgmtDisableInstanceCommand,MgmtDeleteInstanceCommandinInstanceCommands.cs) whoseMgmtprefix 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 | Resolved |
| 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
Resolved 2026-05-16 (commit pending) — confirmed the over-reporting and silent data-loss
branch. Deserialize now returns an OpcUaConfigParseResult (a readonly record struct)
carrying an explicit OpcUaConfigParseStatus (Typed / Legacy / Malformed); genuinely
unparseable input is classified Malformed with IsLegacy == false instead of being
mislabelled as a recoverable legacy row. The struct keeps a custom two-element
Deconstruct(out Config, out bool isLegacy) so the existing SiteRuntime caller
(var (config, _) = ...) is unaffected — verified by a green SiteRuntime build. The XML
doc now describes all three outcomes. Throwing was rejected because the sole consumer
(DeploymentManagerActor.FlattenConnectionConfig) does not wrap the call and is
out-of-scope to change. Regression tests added in OpcUaEndpointConfigSerializerTests
(Deserialize_Malformed_ReportsMalformedNotLegacy, Deserialize_LegacyParsed_StatusIsLegacy,
Deserialize_ObjectWithoutEndpointUrl_ParsesAsLegacy,
Deserialize_TwoElementDeconstruction_StillWorks).
Commons-006 — DynamicJsonElement.TryConvert reports success for unconvertible target types
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| 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
Resolved 2026-05-16 (commit pending) — confirmed by a direct TryConvert regression test
that the original code returned (result: null, true) for an object conversion of a
present value. TryConvert now handles the object target explicitly: it returns
Wrap(_element) (the unwrapped scalar, or the wrapper for objects/arrays) and yields
null only for a genuine JsonValueKind.Null. Non-object targets keep the correct
behavior — a null from ConvertTo now reports false (the dead || typeof(object)
clause is removed) so the binder surfaces a real binding error for unconvertible pairs.
Regression tests added in DynamicJsonElementTests (TryConvert_ObjectTarget_OnPresentValue_ReturnsNonNull,
TryConvert_ObjectTarget_OnJsonNull_ReturnsNull,
TryConvert_NonObjectTarget_OnUnconvertibleValue_ReportsFailure).
Commons-007 — Several Commons types carry non-trivial logic, stretching REQ-COM-6
| Severity | Low |
| Category | Design-document adherence |
| Status | Resolved |
| 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
Resolved 2026-05-16 (commit pending) — design-document decision. This is drift, not a
defect: the helpers are pure, dependency-light (no Akka/ASP.NET/EF, no I/O), and shared
across components, so the intentional choice is to keep them in Commons rather than
duplicate the logic. REQ-COM-6 in docs/requirements/Component-Commons.md was amended
with an explicit "pure-helper carve-out" — stateless, side-effect-free helpers that only
transform/format/parse/validate Commons' own data types are now permitted, with the
qualifying conditions (no I/O dependency, no shared mutable cross-call state, no
orchestration) and the current examples (Result<T>, ScriptParameters, ScriptArgs,
ValueFormatter, DynamicJsonElement, StaleTagMonitor, OpcUaEndpointConfigSerializer,
OpcUaEndpointConfigValidator) spelled out. No regression test — this is a documentation
policy decision. The ArchitecturalConstraintTests heuristic was left as-is since the
carve-out makes these types compliant by design.
Commons-008 — SetConnectionBindingsCommand uses ValueTuple in a wire message contract
| Severity | Low |
| Category | Akka.NET conventions |
| Status | Resolved |
| 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
Resolved 2026-05-16 (commit <pending>) — confirmed against the source: the ValueTuple
was the only positional/tuple element in Messages/ and System.Text.Json serializes it
as Item1/Item2, unfriendly to REQ-COM-5a additive evolution. Introduced a named record
ConnectionBinding(string AttributeName, int DataConnectionId) in
Messages/Management/InstanceCommands.cs (alongside the command, matching the existing
record-per-file convention) and changed SetConnectionBindingsCommand.Bindings to
IReadOnlyList<ConnectionBinding>. All consumers were updated in lock-step: ScadaLink.CLI
(InstanceCommands.TryParseBindings now builds a List<ConnectionBinding>),
ScadaLink.TemplateEngine (InstanceService.SetConnectionBindingsAsync parameter),
ScadaLink.ManagementService (ManagementActor forwards cmd.Bindings unchanged — the
new element type flows through), and one consumer the finding did not list,
ScadaLink.CentralUI (InstanceConfigure.razor's SaveBindings built a List<(string,int)>).
A repo-wide src/ scan confirms no other references remain. Regression tests added in
ConnectionBindingSerializationTests (ConnectionBinding_SerializesWithNamedProperties
asserts named AttributeName/DataConnectionId JSON properties and the absence of
Item1/Item2; SetConnectionBindingsCommand_RoundTripsThroughJson asserts a full
JSON round-trip); existing parse/forward tests (ParseBindings_ValidJson_ReturnsPairs,
SetConnectionBindings_BulkAssignment_Success) were updated to the new type. Affected
suites are green (Commons 226, CLI 77, ManagementService 55, TemplateEngine 287) and
dotnet build ScadaLink.slnx is clean.
Commons-009 — Component-Commons.md is stale relative to the actual file set
| Severity | Low |
| Category | Documentation & comments |
| Status | Resolved |
| 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 onlyIAuditService. - Whole namespaces absent from the REQ-COM-5b folder tree:
Messages/Management,Messages/DataConnection,Messages/Integration,Messages/Instance,Messages/RemoteQuery, plusTypes/DataConnections,Types/Scripts,Serialization/, andValidators/.
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
Resolved 2026-05-16 (commit pending) — docs/requirements/Component-Commons.md was
refreshed against the actual file set. REQ-COM-3 now lists TemplateFolder,
InstanceAlarmOverride and DeployedConfigSnapshot and drops the non-existent
SiteDataConnectionAssignment (no such entity in the code). REQ-COM-4 adds
ISiteRepository (eight repositories). REQ-COM-4a documents IDatabaseGateway,
IExternalSystemClient, IInstanceLocator and INotificationDeliveryService alongside
IAuditService, and corrects the IAuditService owner to the Configuration Database
component. The REQ-COM-5b folder tree was rewritten to include the previously-absent
Types/DataConnections, Types/Flattening, Types/Scripts, the helper types under
Types/, the Messages/DataConnection|Instance|Integration|InboundApi|RemoteQuery|Management
namespaces, and the Serialization/ and Validators/ folders. Documentation-only; no
regression test.
Commons-010 — Behavior-bearing Commons types have no unit tests
| Severity | Low |
| Category | Testing coverage |
| Status | Resolved |
| 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/GetCommandNameround-trip (would have surfaced Commons-004).Result<T>—Match, failure/success accessors, error-on-misuse.OpcUaEndpointConfigSerializertyped↔flat round-trip and legacy fallback.ConfigurationDiff/AlarmContext/ScriptScope— minor, butHasChanges/HasParentlogic 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
Resolved 2026-05-16 (commit pending) — all the listed types now have unit tests.
DynamicJsonElement, ManagementCommandRegistry, Result<T> and the OPC UA serializer
were already covered by tests added in earlier finding fixes (Commons-002/004) and this
pass extended them (Commons-005/006). New focused test files added this pass:
ValueFormatterTests (scalar/collection/null formatting plus the Commons-012
culture-invariance regression), ScriptArgsTests (dictionary / read-only-dictionary /
non-generic-IDictionary / anonymous-object / primitive-rejection paths of
ScriptArgs.Normalize), and FlatteningAndScriptScopeTests (ConfigurationDiff.HasChanges
and ScriptScope.HasParent computed-property logic). The module suite is green at 224
tests (up from 196).
Commons-011 — Result<T>.Failure accepts a null error string
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| 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
Resolved 2026-05-16 (commit pending) — the private failure constructor now calls
ArgumentException.ThrowIfNullOrWhiteSpace(error), so a failed Result<T> always carries
a usable message (ArgumentNullException for null, ArgumentException for empty/
whitespace). The Failure factory XML doc records both exceptions. A repo-wide scan of
.Failure(...) call sites confirmed every caller passes a non-empty literal or
interpolated string, so no consumer is broken. Regression tests added in ResultTests
(Failure_WithNullError_ShouldThrow, Failure_WithBlankError_ShouldThrow).
Commons-012 — ValueFormatter uses current-culture formatting without documenting it
| Severity | Low |
| Category | Documentation & comments |
| Status | Resolved |
| 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
Resolved 2026-05-16 (commit pending) — confirmed ValueFormatter is not UI-only: its
sole consumer is StreamRelayActor, which formats attribute values into outbound gRPC
SiteStreamEvent messages (a wire context), so culture-dependent output was a latent
bug. FormatDisplayValue now formats IFormattable scalars and collection elements with
IFormattable.ToString(null, CultureInfo.InvariantCulture); a new FormatElement helper
applies the same invariant rule per collection element (previously elements went through
the parameterless ToString()). The XML doc gained a remarks block stating the
culture-invariant contract and why. Regression tests added in ValueFormatterTests
(FormatDisplayValue_Double_UsesInvariantCulture_*, _DateTime_*, _CollectionOfDoubles_*,
each pinned under de-DE).
Commons-013 — DynamicJsonElement.TryGetIndex rejects non-int index values
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | src/ScadaLink.Commons/Types/DynamicJsonElement.cs:40-54 |
Description
TryGetIndex accepts an index only when indexes[0] is int index. DynamicJsonElement
is designed for dynamic access from scripts (obj.items[0]). In a dynamic expression the
index operand's runtime type follows the script's variable type — a script that computes
an index in a loop counter or reads it from another DynamicJsonElement (whose numbers
are unwrapped as long by Wrap, see :105) will pass a long, not an int. The
pattern match then fails, TryGetIndex returns false, and the dynamic binder throws a
RuntimeBinderException for what is a perfectly valid in-range index. Because the wrapper
itself surfaces JSON numbers as long, obj.items[obj.count - 1] — count being a wrapped
JSON number — is the exact failing case. The int-only guard also silently rejects
byte/short indices that would widen to a valid array position.
Recommendation
Accept any integral index by converting through Convert.ToInt64 (guarded for
OverflowException) or by matching int, long, short, byte and normalizing to a
single integer before the bounds check. Add a regression test indexing with a long.
Resolution
Resolved 2026-05-17 — confirmed the int-only guard, and during fixing found the
underlying cause was wider than the finding text: Wrap's TryGetInt64 ? long : double
ternary unified both branches to double, so integral JSON numbers were never actually
boxed as long — obj.count always surfaced as double. Fixed at the root: Wrap
delegates to a new WrapNumber helper that boxes integral numbers as long and
non-integral as double with separate typed returns; TryGetIndex now accepts any
integral index via TryGetIntegralIndex (int/long/short/byte/sbyte/ushort/
uint/ulong, plus whole-valued double/decimal) normalized to long before the
bounds check. Internal-only change — no public-API or message-contract change. Regression
tests added in DynamicJsonElementTests (IndexAccess_WithLongIndex_Works,
IndexAccess_WithIndexDerivedFromWrappedJsonNumber_Works,
IndexAccess_WithWideningIntegralIndex_Works, IndexAccess_WithLongIndexOutOfRange_Throws).
Commons-014 — OpcUaEndpointConfigSerializer.Deserialize can mislabel a corrupt typed row as Legacy
| Severity | Low |
| Category | Error handling & resilience |
| Status | Resolved |
| Location | src/ScadaLink.Commons/Serialization/OpcUaEndpointConfigSerializer.cs:107-131 |
Description
Deserialize tries the typed path first: it parses the document, checks for an
endpointUrl property, then calls JsonSerializer.Deserialize<OpcUaEndpointConfig>.
The whole block is wrapped in catch (JsonException) { /* fall through to legacy */ }.
If a row is the current typed shape (it has endpointUrl) but is corrupt in a way that
makes JsonSerializer.Deserialize throw a JsonException — e.g. an enum-valued field
holding an unrecognised string, or a numeric field holding a non-numeric token — the
exception is swallowed and control falls through to LoadLegacy. LoadLegacy only
requires the root to be a JSON object, so it will usually succeed against the same input
and the result is reported as OpcUaConfigParseStatus.Legacy. The Commons-005 fix added
the Malformed status precisely so a caller can tell a recoverable legacy row from
unparseable data; this path re-introduces a softer version of the same confusion — a
genuinely broken current-shape row is presented to the user as a benign "please re-save"
legacy row, and the offending field is silently dropped by FromFlatDict (which ignores
keys it cannot parse) rather than surfaced. The XML doc describes the legacy fallback as
being for "pre-refactor rows" only and does not mention this branch.
Recommendation
Only fall through to LoadLegacy when the typed shape is genuinely not present — i.e.
the endpointUrl property is absent. When endpointUrl is present but typed
deserialization throws, classify the outcome as Malformed (or a distinct status) so the
caller can surface a real error instead of an empty/partial config. Tighten the XML doc
to describe this branch, and add a regression test for a typed row with an invalid enum
field.
Resolution
Resolved 2026-05-17 — confirmed the swallow-and-fall-through against the source. Split
Deserialize into a shape-detection phase and a materialization phase: it first parses
the document to decide whether the row is the current typed shape (root object carrying
endpointUrl); only a non-typed-shape row falls through to LoadLegacy. A row that is
the typed shape but fails JsonSerializer.Deserialize (invalid enum, wrong-typed field)
is now classified Malformed instead of being mislabelled Legacy with the offending
field silently dropped. The OpcUaConfigParseResult shape and (Config, IsLegacy)
deconstruction are unchanged, so existing callers are unaffected. XML docs updated to
describe the corrupt-typed-row branch. Regression tests added in
OpcUaEndpointConfigSerializerTests (Deserialize_TypedShapeWithInvalidEnum_ReportsMalformedNotLegacy,
Deserialize_TypedShapeWithWrongTypeField_ReportsMalformedNotLegacy,
Deserialize_ValidTypedRow_StillReportsTyped).