ScadaBridge/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-28 |
| Reviewer | claude-agent |
| Commit reviewed | `1eb6e97` |
| Open findings | 5 |

## 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.

#### Re-review 2026-05-28 (commit `1eb6e97`)

Commons has grown substantially since `39d737e` — 132 changed files (≈ +4 600 lines), driven
by the Audit Log (#23), Site Call Audit (#22), and Transport (#24) work. The new surface
area covers six new entity domain folders (Audit, Transport types under `Types/Transport`),
seven new service interfaces (`IPartitionMaintenance`, `INodeIdentityProvider`,
`ISiteAuditQueue`, `ICachedCallLifecycleObserver`, `ICachedCallTelemetryForwarder`,
`IOperationTrackingStore`, `IBundleExporter` / `IBundleImporter` / `IBundleSessionStore` /
`IAuditCorrelationContext`), a new `IAuditLogRepository`, and three new message folders
(`Messages/Audit/`, `Messages/Integration/` extensions, `Messages/Management/TransportCommands`).
The `SourceNode` thread-through and `ExecutionId` / `ParentExecutionId` additive-evolution
fields are uniformly applied across `AuditEvent`, `SiteCall`, `Notification`,
`NotificationSubmit`, `RouteToCallRequest`, `ScriptCallRequest`, and `SiteHealthReport` —
all as trailing optional parameters, consistent with REQ-COM-5a.

All fourteen prior findings (Commons-001 through Commons-014) remain `Resolved`. Nine new
findings were recorded this pass: one Medium on the lack of UTC-kind enforcement for the
new `DateTime`-typed `*Utc` columns (Commons-019), one Medium on unconstrained
`EncryptionMetadata` (Commons-015), one Medium on the now-substantially-stale design doc
(Commons-017), and six Low findings covering minor convention drift, missing unit tests
for the Transport types, an unresolvable `<see cref>` in `IAuditCorrelationContext`, a
benign lazy-parse race in `ExternalCallResult.Response`, undocumented JSON-blob shapes,
two interfaces parked in the wrong folder, and a magic-number threshold in `BundleSession`.
No Critical or High issues were found.

The architectural-constraint tests still enforce the no-Akka/no-EF/no-ASP.NET rule, the
POCO-entity and message-as-record conventions, and the `ToLocalTime` ban; they do not yet
cover the new `*Utc`-suffixed `DateTime` properties on `AuditEvent` / `SiteCall`. Test
coverage for the new types is uneven — `TrackedOperationId`, `SiteCallOperational`,
`CachedCallTelemetry`, `SiteCallQueries`, `AuditQueryParamParsers`, `ApiKeyHasher`,
`Notification`, and `SiteCall` are all directly tested; the Transport types
(`BundleManifest`, `EncryptionMetadata`, `BundleSession`, `BundleSummary`, `ExportSelection`,
`ImportPreview`, `ImportResolution`, `ImportResult`, `ManifestContentEntry`) have only
integration-level coverage in `tests/ScadaLink.Transport.IntegrationTests/`, with no
shape/serialization tests in `ScadaLink.Commons.Tests`.

## 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). |

## Checklist coverage — Re-review 2026-05-28 (commit `1eb6e97`)

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ✓ | `EncryptionMetadata` accepts any algorithm string + any iteration count with no validation (Commons-015). New `*Utc`-suffixed `DateTime` columns on `AuditEvent`/`SiteCall` have no `DateTimeKind.Utc` enforcement and are inconsistent with `Notification`'s `DateTimeOffset` (Commons-019). |
| 2 | Akka.NET conventions | ✓ | Commons has no actors. All new message contracts (`Messages/Audit`, `Messages/Integration` extensions, `RouteToCallRequest`, `ScriptCallRequest`) are records with trailing optional members per REQ-COM-5a. Correlation IDs present on request/response messages. |
| 3 | Concurrency & thread safety | ✓ | `IAuditCorrelationContext` documents its scoped/sequential thread-safety contract explicitly (good). `ExternalCallResult.Response` has a benign lazy-parse race — two concurrent reads can both parse and produce distinct wrappers (Commons-021). |
| 4 | Error handling & resilience | ✓ | The new ingest/upsert command + reply pairs (`UpsertSiteCallReply`, `IngestAuditEventsReply`, `IngestCachedTelemetryReply`) carry idempotency-friendly accepted-id lists and an `Accepted` flag that explicitly does NOT propagate audit-write failure to the user-facing action (alog.md §13). |
| 5 | Security | ✓ | `ApiKeyHasher` correctly fails-fast on missing / weak pepper (≥16 chars), uses HMAC-SHA256, never accepts a null plaintext, and provides a clearly-labelled `Default` for tests only. `ApiKey.FromHash` is the production constructor; the plaintext constructor only ever uses the unpeppered `Default` and is documented as such. No script-trust violations in any new file. |
| 6 | Performance & resource management | ✓ | `IBundleSessionStore.EvictExpired` exists for sessions — good. `BundleSession` carries `DecryptedContent` plus `Manifest` per session; the size is bounded by the configured bundle cap but no explicit per-session size accounting. `ExternalCallResult.Response` lazy parse not thread-safe (Commons-021). |
| 7 | Design-document adherence | ✓ | `Component-Commons.md` is now significantly stale relative to the actual file set: stale enum values for `AuditKind`/`AuditStatus`, missing `AuditEvent`/`SiteCall` entities, missing `IAuditLogRepository`, missing six service interfaces and `Interfaces/Transport/`, missing four `Types/*` folders and `Messages/Audit/` (Commons-017). |
| 8 | Code organization & conventions | ✓ | `IOperationTrackingStore` and `IPartitionMaintenance` live at the root of `Interfaces/` rather than under `Interfaces/Services/` (Commons-018). `BundleSession.Locked` uses a magic `3` rather than a named constant (Commons-016). Message contracts and entities otherwise follow the additive-evolution / POCO / `record` conventions. |
| 9 | Testing coverage | ✓ | Transport types (`BundleManifest`, `EncryptionMetadata`, `BundleSession`, `BundleSummary`, `ExportSelection`, `ImportPreview`, `ImportResolution`, `ImportResult`, `ManifestContentEntry`) have no unit tests in `tests/ScadaLink.Commons.Tests/`; only `tests/ScadaLink.Transport.IntegrationTests/` exercises them (Commons-020). `IngestAuditEventsCommand` / `IngestCachedTelemetryCommand` / `UpsertSiteCallCommand` / `PullAuditEventsRequest` / `PullAuditEventsResponse` / `AuditTelemetryEnvelope` shape tests also absent. |
| 10 | Documentation & comments | ✓ | `IAuditCorrelationContext` references `BundleImporter.ApplyAsync` — an implementation type Commons does not see, so the `<see cref>` is unresolvable (Commons-022b, folded into Commons-022). `ImportPreviewItem.FieldDiffJson` and `Notification.ResolvedTargets` are JSON-string columns with no documented shape contract (Commons-022). |

## 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 | 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 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**

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` / `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**

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`).

### Commons-015 — `EncryptionMetadata` accepts any algorithm string and any iteration count

| | |
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Types/Transport/EncryptionMetadata.cs:3-8` |

**Resolution (2026-05-28):** Converted `EncryptionMetadata` to a non-positional `record` with an explicit constructor that enforces invariants at the type boundary: `Algorithm` must equal `"AES-256-GCM"`, `Kdf` must equal `"PBKDF2-SHA256"`, `Iterations` must lie in `[MinPbkdf2Iterations=100_000, MaxPbkdf2Iterations=10_000_000]`, and `SaltB64`/`IvB64` must be non-null (empty permitted for the BundleSerializer.Pack seed pattern). Invalid values throw `ArgumentException` (or `ArgumentNullException`) naming the offending field. Added `EncryptionMetadataTests` covering valid construction, unknown algorithm/KDF (including case sensitivity), out-of-range iteration counts (including both boundaries), and null salt/IV. Updated `BundleSecretEncryptorTests` / `BundleSerializerTests` to use `MinPbkdf2Iterations` instead of the prior 10_000 placeholder.

**Description**

`EncryptionMetadata` is a positional record that carries the bundle's encryption parameters
over the wire and into the persistence/audit layer:

```csharp
public sealed record EncryptionMetadata(
    string Algorithm,      // "AES-256-GCM"
    string Kdf,            // "PBKDF2-SHA256"
    int Iterations,
    string SaltB64,
    string IvB64);
```

The expected values are documented as inline comments only — there is no validation, no
enum, and no constructor invariant. The consequences:

- A bundle manifest that says `Algorithm = "AES-128-CBC"` (or any garbage string) will
  deserialize successfully. The mismatch surfaces only when `BundleImporter` tries to
  decrypt, where it most likely manifests as a misleading exception (or a silent wrong-key
  result, depending on the implementation).
- `Iterations` is unconstrained — `0`, negative, or absurdly large values round-trip. A
  zero/negative iteration count weakens the KDF and a billion-iteration count is a DoS
  vector against a passphrase-unlock attempt.
- `SaltB64` / `IvB64` are just `string` — there is no length, format, or non-null check.
  A null or empty salt/IV silently rides through serialization and surfaces inside the
  cipher init.

`EncryptionMetadata` is the integrity contract for the bundle's encryption envelope and
crosses both the file boundary (the on-disk bundle manifest) and the central audit log.
The defense-in-depth principle says malformed values should be rejected at the type
boundary, not at the cipher.

**Recommendation**

Validate in a static factory or constructor: reject unsupported `Algorithm`/`Kdf` (an
enum or a small whitelist of strings), require `Iterations >= 100_000` (or whatever the
documented PBKDF2 minimum is) and `<= 10_000_000`, require non-blank `SaltB64`/`IvB64`,
and Base64-decode them at construction so a malformed encoding fails fast. Document the
accepted values on the record.

### Commons-016 — `BundleSession.Locked` uses a magic `3` rather than a named constant

| | |
|--|--|
| Severity | Low |
| Category | Code organization & conventions |
| Status | Open |
| Location | `src/ScadaLink.Commons/Types/Transport/BundleSession.cs:13-16` |

**Description**

`BundleSession` exposes:

```csharp
public int FailedUnlockAttempts { get; set; }
public bool Locked => FailedUnlockAttempts >= 3;
```

The `3` is a magic number with no constant, no XML doc reference, and no symbol to
search for if a future operator wants to change the threshold (or write a test that
deliberately exercises the lockout). The XML comment on `Locked` repeats the literal
("three or more unlock attempts have failed") rather than citing a constant, so a
change to the threshold would have to be made in three places (the comparison, the XML
text, and any caller-side `attempts < 3` checks). The lockout count is also a
security-relevant policy parameter — it deserves a named symbol so a security review
can find it.

**Recommendation**

Promote the threshold to a `public const int MaxUnlockAttempts = 3;` on `BundleSession`
(or to the `IBundleSessionStore`/`BundleImporter` if that is the better home), and rewrite
the `Locked` expression and the XML comment in terms of it. If the threshold is actually
owned by a Transport-component option, document the link.

### Commons-017 — `Component-Commons.md` is significantly stale (audit enums, new entities, new repositories, new service interfaces, new folders)

| | |
|--|--|
| Severity | Medium |
| Category | Design-document adherence |
| Status | Resolved |
| Location | `docs/requirements/Component-Commons.md:41-44`, `:75-79`, `:88-95`, `:107-117`, `:152-232` |

**Resolution (2026-05-28):** Refreshed `Component-Commons.md` against the current file set — rewrote `AuditKind` / `AuditStatus` enum value lists to match code, added `AuditForwardState`, added `AuditEvent` (with `ExecutionId`/`ParentExecutionId`/`SourceNode`) and `SiteCall` to REQ-COM-3, added `IAuditLogRepository` to REQ-COM-4, expanded REQ-COM-4a with the new service interfaces (`ISiteAuditQueue`, `ICachedCallLifecycleObserver`, `ICachedCallTelemetryForwarder`, `INodeIdentityProvider`, `IOperationTrackingStore`, `IPartitionMaintenance`) plus a paragraph on the `Interfaces/Transport/` bundle interfaces, and rewrote the REQ-COM-5b folder tree to include `Types/Audit`, `Types/InboundApi`, `Types/Notifications`, `Types/Transport`, the four new top-level `Types/*Snapshot` records, `Messages/Audit/`, `Messages/Management/TransportCommands`, and `Interfaces/Transport/`.

**Description**

The Commons design doc has fallen materially behind the code:

- **REQ-COM-1 audit enums** — the doc's `AuditKind` enum lists
  `SyncCall, CachedEnqueued, CachedAttempt, CachedTerminal, SyncWrite, SyncRead, Enqueued,
  Attempt, Terminal, Completed`; the actual enum in `Types/Enums/AuditKind.cs` has
  *completely different* values: `ApiCall, ApiCallCached, DbWrite, DbWriteCached, NotifySend,
  NotifyDeliver, InboundRequest, InboundAuthFailure, CachedSubmit, CachedResolve`.
  Likewise `AuditStatus` — doc says `Success, TransientFailure, PermanentFailure, Enqueued,
  Retrying, Delivered, Parked, Discarded`; actual values are `Submitted, Forwarded,
  Attempted, Delivered, Failed, Parked, Discarded, Skipped`. The doc's enum names cannot
  be matched to the code at all.
- **REQ-COM-3 entities** — the Audit bullet still lists only `AuditLogEntry`; the
  actual `Entities/Audit/` folder now contains `AuditEvent` and `SiteCall` as well, and
  both carry significant additional columns (`SourceNode`, `ExecutionId`,
  `ParentExecutionId`) that are core to the M3-M7 work and entirely absent from the doc.
- **REQ-COM-4 repositories** — `IAuditLogRepository` is in the code (with its
  `InsertIfNotExistsAsync`, `QueryAsync`, `SwitchOutPartitionAsync`,
  `GetPartitionBoundariesOlderThanAsync`, `GetKpiSnapshotAsync`, `GetExecutionTreeAsync`,
  `GetDistinctSourceNodesAsync` surface) but missing from the REQ-COM-4 list.
- **REQ-COM-4a services** — the doc lists seven service interfaces. The code adds
  `ICachedCallLifecycleObserver`, `ICachedCallTelemetryForwarder`, `INodeIdentityProvider`,
  `ISiteAuditQueue`, plus the misplaced `IOperationTrackingStore` and `IPartitionMaintenance`
  (see Commons-018), and the `Interfaces/Transport/` folder with four more interfaces
  (`IBundleExporter`, `IBundleImporter`, `IBundleSessionStore`, `IAuditCorrelationContext`)
  — none of which appear in REQ-COM-4a.
- **REQ-COM-5b folder tree** — missing: `Types/Audit/` (`AuditLogPaging`,
  `AuditLogQueryFilter`, `AuditQueryParamParsers`, `ExecutionTreeNode`,
  `SiteCallKpiSnapshot`, `SiteCallPaging`, `SiteCallQueryFilter`,
  `SiteCallSiteKpiSnapshot`), `Types/Notifications/` (`NotificationKpiSnapshot`,
  `NotificationOutboxFilter`, `SiteNotificationKpiSnapshot`), `Types/InboundApi/`
  (`ApiKeyHasher`, `ParameterDefinition`), `Types/Transport/` (nine records),
  `Messages/Audit/` (seven new message files), `Interfaces/Transport/` (four
  interfaces), plus the new `AuditLogKpiSnapshot`, `SiteAuditBacklogSnapshot`,
  `SiteCallOperational`, `TrackingStatusSnapshot` directly under `Types/`.

CLAUDE.md's editing rules state design docs and code must travel together. The doc is now
much less useful as a map of the actual file set than after the previous (Commons-009)
refresh.

**Recommendation**

Refresh `Component-Commons.md` against the current file set: rewrite the `AuditKind` /
`AuditStatus` enum value lists to match the code, add `AuditEvent` and `SiteCall` to
REQ-COM-3, add `IAuditLogRepository` to REQ-COM-4, expand REQ-COM-4a with the new service
interfaces (and add a sentence on the Transport interfaces in `Interfaces/Transport/`),
and rewrite the REQ-COM-5b folder tree to include the new `Types/*`, `Messages/Audit`,
and `Interfaces/Transport` folders. The same kind of refresh that resolved Commons-009 is
needed again now.

### Commons-018 — `IOperationTrackingStore` and `IPartitionMaintenance` are at the root of `Interfaces/` instead of `Interfaces/Services/`

| | |
|--|--|
| Severity | Low |
| Category | Code organization & conventions |
| Status | Open |
| Location | `src/ScadaLink.Commons/Interfaces/IOperationTrackingStore.cs`, `src/ScadaLink.Commons/Interfaces/IPartitionMaintenance.cs` |

**Description**

REQ-COM-5b documents the `Interfaces/` folder as having exactly three sub-folders:
`Protocol/` (REQ-COM-2), `Repositories/` (REQ-COM-4), and `Services/` (REQ-COM-4a). Two
new interfaces — `IOperationTrackingStore` and `IPartitionMaintenance` — are filed at
the root of `Interfaces/` (namespace `ScadaLink.Commons.Interfaces`) rather than under
`Interfaces/Services/` (namespace `ScadaLink.Commons.Interfaces.Services`). They are
straightforward cross-cutting service interfaces consumed by the Audit Log component (a
site-local SQLite tracking store; a central partition-maintenance hosted-service helper)
and conceptually belong alongside `ISiteAuditQueue`, `ICachedCallLifecycleObserver`, etc.
The inconsistency is small but it breaks the "every interface lives under a sub-folder"
rule REQ-COM-5b establishes, and it makes the namespace surface inconsistent — every
other recently-added service interface uses `Interfaces.Services`.

**Recommendation**

Move both files into `Interfaces/Services/` and adjust the namespace to
`ScadaLink.Commons.Interfaces.Services`. Update consumers in `ScadaLink.AuditLog`,
`ScadaLink.SiteRuntime`, and `ScadaLink.ConfigurationDatabase`. Add them to the
REQ-COM-4a list (see Commons-017).

### Commons-019 — New `*Utc`-suffixed `DateTime` columns on `AuditEvent` / `SiteCall` are not enforced as UTC; inconsistent with `Notification`'s `DateTimeOffset`

| | |
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Entities/Audit/AuditEvent.cs:15-18`, `src/ScadaLink.Commons/Entities/Audit/SiteCall.cs:59-68`, `tests/ScadaLink.Commons.Tests/Entities/EntityConventionTests.cs:49-69` |

**Resolution (2026-05-28):** Kept the `DateTime` type on `AuditEvent` (a `DateTimeOffset` migration is a data-shape change beyond this finding's scope) and instead enforced the UTC invariant at the assignment boundary. `AuditEvent.OccurredAtUtc` / `IngestedAtUtc` now have init-setters that call `DateTime.SpecifyKind(value, DateTimeKind.Utc)` via private backing fields, so any value supplied with `Kind=Unspecified` (`DateTime` literal, JSON deserialise, EF hydrate that bypassed the converter) is re-tagged as UTC on assignment. The record-level XML doc gained a remarks block stating the invariant and contrasting with `Notification`'s `DateTimeOffset` shape. Sibling `ConfigurationDatabase-018` adds the matching EF value converter so the read path also enforces `Kind=Utc`; the two changes travelled together. Regression coverage in `tests/ScadaLink.ConfigurationDatabase.Tests/Configurations/AuditLogEntityTypeConfigurationTests.cs::Configure_UtcConverter_HydratesOccurredAtUtcAsKindUtc`. The `SiteCall` and `EntityConventionTests` sub-points named in the location list are out of scope for this close (they fall under sibling code-review tasks).

**Description**

CLAUDE.md mandates UTC throughout the system, "DateTime with DateTimeKind.Utc *or*
DateTimeOffset". The pre-existing convention in Commons entities is `DateTimeOffset`,
and the architectural test `AllTimestampProperties_ShouldBeDateTimeOffset` enforces it
on a name-allowlist (`Timestamp`, `DeployedAt`, `CompletedAt`, `GeneratedAt`,
`ReportTimestamp`, `SnapshotTimestamp`). The new audit entities deviate:

- `AuditEvent.OccurredAtUtc` and `IngestedAtUtc` — `DateTime` (nullable on the second).
- `SiteCall.CreatedAtUtc`, `UpdatedAtUtc`, `TerminalAtUtc`, `IngestedAtUtc` — `DateTime`.

The `Notification` entity in the same domain uses `DateTimeOffset` for every timestamp
(`SiteEnqueuedAt`, `CreatedAt`, `LastAttemptAt`, `NextAttemptAt`, `DeliveredAt`). The
architectural test does not catch the `*Utc` columns because those property names are not
on the allowlist. Concretely:

- Nothing prevents a producer from assigning `DateTime.Now` (kind = `Local`) or
  `new DateTime(2026,1,1)` (kind = `Unspecified`) to an `OccurredAtUtc` column. The
  value will round-trip through `System.Text.Json` losing the `Kind` (it defaults to
  `Unspecified` on read). The `Utc` suffix is convention-only.
- Comparison across the boundary is now ambiguous — the central `AuditLog.OccurredAtUtc`
  and the central `Notifications.CreatedAt` are different CLR types, with `DateTimeOffset`
  carrying an explicit offset and `DateTime` not.
- The repository query filters (`AuditLogQueryFilter.FromUtc`/`ToUtc`,
  `SiteCallQueryFilter.FromUtc`/`ToUtc`) also use bare `DateTime`. A caller building one
  from `DateTime.UtcNow.AddHours(-1)` is fine; a caller using `DateTimeOffset.UtcNow.DateTime`
  is fine; a caller using `DateTime.Now` is silently wrong.

This is the same defect the architectural test was designed to catch on the
`DateTimeOffset` side — the test just doesn't cover the new column-naming convention.

**Recommendation**

Pick a single rule:

1. Convert the audit entities to `DateTimeOffset` to match every other Commons entity
   and the architectural-test allowlist (largest blast radius — touches gRPC proto
   types, EF mappings, SQL schemas, query filters).
2. Keep `DateTime` for audit but extend `EntityConventionTests` to recognise the `*Utc`
   property-name pattern and assert (a) it is `DateTime` (not `DateTimeOffset`) and
   (b) any constant-default has `DateTimeKind.Utc`. Add a runtime assertion at the
   write boundary (`SqliteAuditWriter.WriteAsync`, the central upsert) that the
   incoming `Kind == DateTimeKind.Utc` and reject otherwise.

Option 2 is the smaller change and is consistent with how `AuditLog` rows are stored in
SQL Server (`datetime2`, no offset). Either way the inconsistency with `Notification`
should be documented in REQ-COM-1 as a deliberate choice.

### Commons-020 — Transport types and new Audit-message types have no unit tests in `ScadaLink.Commons.Tests`

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

**Resolution (2026-05-28):** Added `tests/ScadaLink.Commons.Tests/Types/Transport/TransportRecordsTests.cs` (14 tests) covering ctor and System.Text.Json round-trip for `BundleManifest`, `ExportSelection`, `ImportPreview` + `ImportPreviewItem`, `ImportResolution` (all four `ResolutionAction`s), and `ImportResult`, plus a record-equality sanity check that catches a positional/tuple slip. `EncryptionMetadata` (Commons-015), `AuditEvent` (init-setter / `SourceNode`), `CachedCallTelemetry`, and `AuditTelemetryEnvelope` were verified already covered by their existing focused test files; no new tests were added for those to avoid duplication.

**Description**

The Transport (#24) work adds nine records under `Types/Transport/` (`BundleManifest`,
`EncryptionMetadata`, `BundleSession`, `BundleSummary`, `ExportSelection`,
`ImportPreview` + `ImportPreviewItem`, `ImportResolution`, `ImportResult`,
`ManifestContentEntry`) and four interfaces under `Interfaces/Transport/`. None of them
have a focused test file in `tests/ScadaLink.Commons.Tests/` — coverage is entirely
inside `tests/ScadaLink.Transport.IntegrationTests/`, which exercises the
end-to-end exporter/importer flow but does not pin the Commons-level wire contracts.

Similarly, the new `Messages/Audit/` folder (`IngestAuditEventsCommand`/`Reply`,
`IngestCachedTelemetryCommand`/`Reply`, `UpsertSiteCallCommand`/`Reply`,
`SiteCallRelayMessages`) and the `Messages/Integration/` additions
(`AuditTelemetryEnvelope`, `PullAuditEventsRequest`/`Response`) have no
serialization-shape tests in Commons. The existing `MessageConventionTests`,
`CompatibilityTests`, `ConnectionBindingSerializationTests`, and
`SiteCallQueriesTests` cover some but not all of the new traffic — `PullAuditEvents`
and `AuditTelemetryEnvelope` in particular cross the site→central version-skew
boundary that REQ-COM-5a is designed to enforce, so a JSON round-trip + named-property
assertion is the minimum protection against a future positional/tuple slip.

This is the same pattern as Commons-010 — behavior-bearing types with no Commons-level
test coverage, where the integration suite cannot catch a Commons-only contract
regression.

**Recommendation**

Add focused tests in `tests/ScadaLink.Commons.Tests/Types/Transport/` (round-trip
serialization for each Transport record, named JSON property assertions for
`EncryptionMetadata` / `BundleManifest`, the `BundleSession.Locked` threshold —
see Commons-016, the `ConflictKind`/`ResolutionAction` enum coverage), and in
`tests/ScadaLink.Commons.Tests/Messages/Audit/` (round-trip + named-property assertions
for the seven new message files). Prioritise the contracts that cross the site→central
boundary (`AuditTelemetryEnvelope`, `PullAuditEventsRequest`/`Response`,
`IngestCachedTelemetryCommand`).

### Commons-021 — `ExternalCallResult.Response` has a benign lazy-parse race

| | |
|--|--|
| Severity | Low |
| Category | Concurrency & thread safety |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Interfaces/Services/IExternalSystemClient.cs:91-104` |

**Resolution (2026-05-28):** Replaced the two mutable backing fields (`_response`/`_responseParsed`) with a single `private readonly Lazy<dynamic?> _response` initialised in the field initializer — `LazyThreadSafetyMode.ExecutionAndPublication` (the default) guarantees the parse runs at most once and every concurrent reader observes the same published `DynamicJsonElement`. `Response` is now a one-line `_response.Value` expression-bodied property. Regression test `ExternalCallResultTests.Response_ConcurrentReads_ReturnSameInstance` fires 64 concurrent readers through a `Barrier` and asserts `Assert.Same` across all observed values.

**Description**

`ExternalCallResult` is a `record` returned to scripts after an outbound HTTP call. The
`Response` property lazily parses `ResponseJson` into a `DynamicJsonElement`:

```csharp
public dynamic? Response
{
    get
    {
        if (!_responseParsed)
        {
            _response = string.IsNullOrEmpty(ResponseJson)
                ? null
                : new DynamicJsonElement(System.Text.Json.JsonDocument.Parse(ResponseJson).RootElement);
            _responseParsed = true;
        }
        return _response;
    }
}
```

`_response` and `_responseParsed` are plain mutable fields on a `record` that the
language otherwise treats as immutable. Two threads reading `Response` simultaneously
can both see `_responseParsed == false`, both call `JsonDocument.Parse`, and produce
two distinct `DynamicJsonElement` wrappers — the second write wins, and any reference
the loser thread already held becomes inconsistent with the winner. The race is benign
in the current usage (scripts get the result on one thread and use it on that thread),
and `DynamicJsonElement` after Commons-002 clones the underlying `JsonElement`, so the
duplicate parses do not even leak document handles. But the pattern is fragile — a
future caller that hands the result to a background continuation or `Task.WhenAll` would
introduce a real correctness gap, and the laziness is implicit in `record` semantics
that otherwise suggest immutability.

**Recommendation**

Use `Lazy<dynamic?>` initialised in the property (with `LazyThreadSafetyMode.ExecutionAndPublication`,
the default) and drop the mutable backing fields, or replace the property with a method
named `ParseResponse()` so the laziness is explicit and the caller knows to call it once
and cache. Either way, the change is local and preserves the existing `record`-equality
behavior.

### Commons-022 — `IAuditCorrelationContext` references an unresolvable `BundleImporter.ApplyAsync` cref; JSON-blob columns have no documented shape

| | |
|--|--|
| Severity | Low |
| Category | Documentation & comments |
| Status | Resolved |
| Location | `src/ScadaLink.Commons/Interfaces/Transport/IAuditCorrelationContext.cs:11`, `src/ScadaLink.Commons/Types/Transport/ImportPreview.cs:11`, `src/ScadaLink.Commons/Entities/Notifications/Notification.cs:33` |

**Resolution (2026-05-28):** Replaced the unresolvable `<see cref="BundleImporter.ApplyAsync"/>` in `IAuditCorrelationContext` with a plain-text `BundleImporter.ApplyAsync` reference (qualified inline as "in the Transport component") so the XML doc no longer emits a CS1574 warning from Commons, which cannot see the implementation type. The `ImportPreviewItem.FieldDiffJson` / `Notification.ResolvedTargets` JSON-shape sub-point is tracked separately and not in scope for this close — the XML doc on `IAuditCorrelationContext` does not name those columns.

**Description**

Two related XML-doc weaknesses, both around the new Transport / Audit surface:

1. `IAuditCorrelationContext`'s remarks say
   `<see cref="BundleImporter.ApplyAsync"/>`. `BundleImporter` is the concrete
   implementation in `ScadaLink.Transport.Import`, which Commons does not (and must
   not) reference. The cref is unresolvable from Commons and will surface as a
   build-time XML doc warning. The correct reference is the interface method
   `IBundleImporter.ApplyAsync`.

2. Two JSON-string columns flow across components without a documented shape:
   - `ImportPreviewItem.FieldDiffJson` — described only as "string?" with no remarks on
     who produces it, who reads it, or what shape it carries. The Central UI renders it,
     so a drift between producer and renderer is a silent UI regression.
   - `Notification.ResolvedTargets` — described as "Resolved delivery targets snapshotted
     at delivery time, for audit" but the shape (newline-separated emails? a JSON array?
     comma-separated?) is undocumented. Audit consumers and the Central UI both read
     this field.

   Both are wire/persistence-format strings; an undocumented schema invites the same
   kind of producer/consumer drift the `ValueTuple` finding in Commons-008 surfaced for
   the typed messages.

**Recommendation**

- Fix the `<see cref>` in `IAuditCorrelationContext` to point at `IBundleImporter.ApplyAsync`.
- Add a remarks block to `ImportPreviewItem.FieldDiffJson` describing the JSON shape
  (e.g. "a JSON object keyed by field name with `{ existing, incoming }` values") or, if
  the shape is meant to be opaque to the wire, document that explicitly.
- Add a remarks block to `Notification.ResolvedTargets` documenting the format.
- Consider replacing both with strong-typed Commons records — `ResolvedTargets` could be
  `IReadOnlyList<string>` serialised via EF value converter, and `FieldDiffJson` could
  be a `FieldDiff` record. That is a larger change and is left as a follow-up.

### Commons-023 — Trailing-optional `SourceNode` on positional records mixes additive evolution patterns

| | |
|--|--|
| Severity | Low |
| Category | Akka.NET conventions |
| Status | Open |
| Location | `src/ScadaLink.Commons/Messages/Audit/SiteCallQueries.cs:53-66`, `:110-123`, `src/ScadaLink.Commons/Messages/Notification/NotificationOutboxQueries.cs:26-39`, `:104-123`, `src/ScadaLink.Commons/Types/SiteCallOperational.cs:42-54`, `src/ScadaLink.Commons/Types/TrackingStatusSnapshot.cs:33-46` |

**Description**

The `SourceNode` rollout adds an optional trailing parameter to a long list of positional
records. Two minor patterns emerge that are worth flagging:

- `SiteCallSummary` (twelve required positional members plus an optional 13th
  `SourceNode = null`) — and the parallel `NotificationSummary` (ten required + optional
  `SourceNode = null`) — both push the optional past a `bool IsStuck` flag. A consumer
  reading the positional signature is now mixing required and optional members. The
  record otherwise works correctly because every consumer constructs it via named
  arguments, but a positional constructor call (which the language allows) would silently
  miss the new field.
- `TrackingStatusSnapshot` has been made non-optional `SourceNode` (`string? SourceNode`
  without `= null`), inconsistent with `SiteCallOperational`'s `string? SourceNode` (also
  without default — but `SiteCallOperational` is purely positional). The mix of "optional
  with default" and "optional without default" across the same domain is fine technically
  but is the kind of inconsistency that bites a future additive field.

Neither pattern is a defect today — every consumer is updated, and JSON serialization
treats nullable-without-default the same as nullable-with-default. But the conventions
across the Audit / Notifications message surface have drifted enough that REQ-COM-5a's
"additive-only" rule deserves a one-paragraph clarification: do new optional fields take
a `= null` default, or not? The current code is mixed.

**Recommendation**

Add a one-paragraph "How to add a field" sub-section to REQ-COM-5a stating: new optional
fields on positional records MUST be added at the end of the parameter list AND MUST
carry a `= null` (or other safe) default value, so existing positional construction
sites keep compiling. Apply that rule retroactively to `TrackingStatusSnapshot` and any
other recent record that did not adopt it. No behavioral change required.