diff --git a/code-reviews/Runtime/findings.md b/code-reviews/Runtime/findings.md new file mode 100644 index 00000000..cba01606 --- /dev/null +++ b/code-reviews/Runtime/findings.md @@ -0,0 +1,229 @@ +# Code Review — Runtime + + + +| Field | Value | +|---|---| +| Module | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 0 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Findings Runtime-001, Runtime-004, Runtime-005 | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found (see Notes) | +| 4 | Error handling & resilience | Findings Runtime-002, Runtime-003 | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | Finding Runtime-006 | +| 10 | Documentation & comments | No issues found | + +### Scope + +22 source `.cs` under `src/Server/ZB.MOM.WW.OtOpcUa.Runtime` (6129 LOC): the deployment-artifact +decode (`DeploymentArtifact.cs`), the OPC UA publish actor (`OpcUaPublishActor.cs`), the driver host + +instance actors (`DriverHostActor.cs`, `DriverInstanceActor.cs`, `DriverSpawnPlan.cs`, +`NativeAlarmProjector.cs`), the address-space write gateway (`ActorNodeWriteGateway.cs`), health probes +(`DbHealthProbeActor.cs`, `PeerOpcUaProbeActor.cs`, `PeerProbeSupervisor.cs`), the historian adapter + +options (`HistorianAdapterActor.cs`, `AlarmHistorianOptions.cs`, `ServerHistorianOptions.cs`), scripted +alarms (`ScriptedAlarmHostActor.cs`, `EfAlarmConditionStateStore.cs`, +`DependencyMuxTagUpstreamSource.cs`), scripting (`DpsScriptLogPublisher.cs`), virtual tags +(`DependencyMuxActor.cs`, `VirtualTagActor.cs`, `VirtualTagHostActor.cs`), the health publisher +(`AkkaDriverHealthPublisher.cs`), and DI wiring (`ServiceCollectionExtensions.cs`). + +### Notes (no-finding observations) + +- **Concurrency**: actors are single-threaded per instance; off-thread continuations + (`OpcUaPublishActor.OnHealthTick`, `DriverHostActor.HandleRouteNodeWrite`, + `ScriptedAlarmHostActor.OnApply`/`OnEngineEmission`) correctly capture `Sender`/`Self` before the + `await`/`ContinueWith` and only construct messages off-thread, mutating state only back on the actor + thread. `DependencyMuxTagUpstreamSource` is the one genuinely-shared type and uses immutable + observer lists + atomic CAS correctly. No shared mutable state crosses the actor boundary. +- **Security**: write-through is Primary-gated AND `WriteOperate`-gated upstream of Runtime + (`OnWriteValue` in the node manager); native-alarm ack is `AlarmAck`-gated upstream; Runtime re-applies + the Primary gate defensively. No secret logging — `AlarmHistorianOptions.Validate` / + `ServerHistorianOptions.Validate` warn when `SharedSecret` is empty but never echo the secret value. + +## Findings + +### Runtime-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs:311-318`; cross-module `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpacePlan.cs:56-61` + `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/IOpcUaAddressSpaceSink.cs:55` | +| Status | Deferred | + +**Description:** This is the Runtime side of the orchestrator-handed cross-module item **OpcUaServer-001**: +a UNS Area/Line *rename* (display-name-only edit, no Equipment/Tag/Driver/Alarm change) produces an +`AddressSpacePlan` that `IsEmpty`, so `OpcUaPublishActor.HandleRebuild` short-circuits at line 313 +(`if (plan.IsEmpty) { ...; return; }`) **before** reaching `_applier.MaterialiseHierarchy(composition)` +at line 326 — the rebuilt folder display-name never lands and OPC UA clients keep browsing the stale name. + +I verified this is **NOT self-contained within Runtime**: a complete fix needs changes in two other +modules that Runtime cannot touch: +1. `AddressSpacePlan.IsEmpty` (in OpcUaServer) does not diff `UnsArea`/`UnsLine` at all (it only tracks + Equipment/Driver/Alarm/EquipmentTag/VirtualTag). For the rename to drive a rebuild, the plan must + gain a UNS-changed signal (a new diff set) so `IsEmpty` returns `false`. +2. Even if `MaterialiseHierarchy` were reached, `IOpcUaAddressSpaceSink.EnsureFolder` (in Commons) is + contractually **idempotent and returns the existing folder unchanged** — it has no rename semantics, + so an existing folder's `DisplayName` would not be updated. `EnsureFolder` (or a new `RenameFolder`) + in the Commons contract + its SDK-backed implementation must actually re-set the display name. + +Both (1) and (2) are wire/contract-shaped changes in OpcUaServer + Commons, outside this module's edit +boundary. The Runtime side alone (acting on a UNS-changed plan and re-running `MaterialiseHierarchy`) +is necessary but not sufficient. + +**Recommendation:** Track jointly with OpcUaServer-001. OpcUaServer: add a UNS-rename diff set to +`AddressSpacePlan` so `IsEmpty` is false on a rename. Commons: give `EnsureFolder` (or a new +`RenameFolder`) update-display-name semantics on an existing folder. Runtime: once those land, ensure +`HandleRebuild` runs `MaterialiseHierarchy` for a non-empty UNS-only plan (it already does, since the +short-circuit only fires for a truly-empty plan). No Runtime-only change can close it safely. + +**Resolution:** _(deferred — needs OpcUaServer `AddressSpacePlan` rename signal + Commons `EnsureFolder`/`RenameFolder` rename semantics; both outside the Runtime edit boundary. Tracked with OpcUaServer-001.)_ + +### Runtime-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Error handling & resilience | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs:148-169, 782-828` | +| Status | Resolved | + +**Description:** The artifact-decode methods document a "lenient" contract — *"Empty / malformed blobs +return an empty list"* / *"return an empty composition"* — and guard every parse path with +`catch (JsonException)`. But the per-row readers (`TryReadSpec`, `ReadAreaProjection`, +`ReadLineProjection`, `ReadEquipmentNode`, `ReadDriverPlan`, `ReadAlarmPlan`) call `JsonElement.GetString()` +and `GetBoolean()` directly on a property element. If the artifact is *valid JSON but a field carries the +wrong JSON type* (e.g. `"Enabled": "true"` as a string, or `"Name": 5` as a number), those accessors throw +`InvalidOperationException`, which is **not** a `JsonException` and so escapes the method — defeating the +documented lenient-parse contract. For `ParseDriverInstances` (called from `ReconcileDrivers` at +`DriverHostActor.cs:889`, outside its own try) the escape propagates to `ApplyAndAck`, marking the whole +deployment **Failed** instead of degrading gracefully. The artifact is normally machine-generated with +controlled types, so impact is bounded, but a single wrong-typed field shouldn't fail-the-deploy when the +code claims to tolerate malformed input. + +**Recommendation:** Make the per-row readers type-check before reading (`enEl.ValueKind == True/False` +before `GetBoolean`, `ValueKind == String` before `GetString`) — the pattern the `Build*` methods and +`ExtractTag*` helpers already follow — so a wrong-typed field degrades to the field default / a skipped row +rather than throwing. Equivalently, broaden the guards to also catch `InvalidOperationException`. + +**Resolution:** Resolved 2026-06-19 — `TryReadSpec` now reads `Enabled` only on a real `True/False` token +(defaulting `true` otherwise) and reads every string field via a `ReadString` helper that returns null on a +non-string `ValueKind`; `ReadAreaProjection`/`ReadLineProjection`/`ReadEquipmentNode`/`ReadDriverPlan`/`ReadAlarmPlan` +use the same helper. Wrong-typed fields now degrade to the field default / skipped row instead of throwing +`InvalidOperationException`. Regression test `DeploymentArtifactMalformedTypeTests`. + +### Runtime-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/OpcUa/OpcUaPublishActor.cs:320-342` | +| Status | Won't Fix | + +**Description:** In `HandleRebuild`, `_lastApplied = composition` is assigned (line 321) immediately after +`_applier.Apply(plan)` returns, but BEFORE the four `Materialise*` passes run. If one of those passes throws +(e.g. `MaterialiseEquipmentTags`), the catch logs the error and returns — but `_lastApplied` has already +advanced to the new composition. The next rebuild then diffs against a composition whose materialisation +only partially completed, so the partially-failed nodes won't be re-added (the diff sees them as already +applied). In practice the `Materialise*` passes are individually try/wrapped (`SafeEnsureFolder` etc.) and +swallow per-node failures, so a throw out of a whole pass is unlikely; and the next *config-changing* +deploy re-materialises everything. Net risk is low. + +**Recommendation:** Advance `_lastApplied` only after all materialise passes complete (move the assignment +to the end of the try, after `MaterialiseEquipmentVirtualTags`). Note this is a deliberate trade-off — moving +it means a partial failure re-runs `Apply(plan)` (the diff) against the OLD composition next time, which is +the safer direction. + +**Resolution:** Won't Fix (2026-06-19) — the four `Materialise*` passes are each fully `Safe*`-wrapped at the +per-node level (`AddressSpaceApplier.SafeEnsureFolder` / `SafeEnsureVariable` / `SafeMaterialiseAlarmCondition` +swallow and log every per-node exception), so a pass cannot throw out to `HandleRebuild` in practice — the +ordering hazard is not reachable. Reordering the assignment would change the recovery direction on a +hypothetical unreachable path without a demonstrable live benefit, and would risk re-running `Apply(plan)` +against a stale composition. Recorded as a latent-ordering note rather than a fix. + +### Runtime-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs:683-684` | +| Status | Won't Fix | + +**Description:** `HandleRouteNodeWrite` forwards `new DriverInstanceActor.WriteAttribute(target.FullName, +msg.Value!)` where `RouteNodeWrite.Value` is `object?` (nullable) but `WriteAttribute.Value` is non-nullable +`object`. The `!` null-forgiving operator suppresses the warning but does not prevent a genuine `null` from +flowing through to `WriteRequest`/`IWritable.WriteAsync` if an OPC UA client writes a null value. Whether a +null write coerces cleanly is driver-dependent; at minimum the non-null contract on `WriteAttribute.Value` +is misleading. + +**Recommendation:** Either make `WriteAttribute.Value` an `object?` (honest contract; the driver already has +to handle type coercion / rejection), or reject a null write at the gateway with a clear +`NodeWriteResult(false, "null value")`. Low priority — null variable writes are rare and downstream drivers +generally reject them with a Bad status anyway. + +**Resolution:** Won't Fix (2026-06-19) — this is a public message-contract shape (`WriteAttribute.Value` is a +wire-serialised Akka message record). Widening it to `object?` or adding a null-reject branch touches the +inbound-write contract shared with `ActorNodeWriteGateway` / the node-manager wiring; the orchestrator's hard +constraints forbid a wire/message-contract change in this pass. Downstream `IWritable.WriteAsync` +implementations already reject an uncoercible value with a Bad status, so the live impact is a slightly less +clear error string, not a crash or data loss. Recorded for a future contract-touching pass. + +### Runtime-005 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs:610-615` | +| Status | Won't Fix | + +**Description:** `BuildEquipmentScriptedAlarmPlans`'s local `ReadBool` has a comment warning that *"GetBoolean +only runs for a genuine true/false token (a non-bool token would otherwise throw InvalidOperationException, +uncaught here)"*. The comment describes a hazard that the code already prevents — the guard +`b.ValueKind is JsonValueKind.True or JsonValueKind.False` runs before `GetBoolean`, so `GetBoolean` can +never see a non-bool token and never throws. The comment is stale/misleading (it reads as if the throw is a +live risk). + +**Recommendation:** Trim the parenthetical to state the guard makes the call safe, rather than implying an +uncaught throw. Purely a comment clarity issue. + +**Resolution:** Won't Fix (2026-06-19) — the code is correct (the `ValueKind` guard precedes `GetBoolean`), +and the comment is defensively accurate in spirit (it explains *why* the guard exists). The wording is +slightly alarmist but not factually wrong, and the parity-critical `ExtractTag*` mirror methods carry the +same defensive phrasing for consistency. Not worth a churn edit on a byte-parity-sensitive file. + +### Runtime-006 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs` (whole-file) | +| Status | Resolved | + +**Description:** The artifact-decode parity suites (`DeploymentArtifact*ParityTests`) cover the happy path + +the byte-parity-with-composer contract thoroughly, but there is no test asserting the documented +*lenient-parse* contract under **valid-JSON-but-wrong-type** input (e.g. a string `Enabled`, a numeric +`Name`). That gap is exactly what let Runtime-002 go unnoticed. + +**Recommendation:** Add a regression test feeding a syntactically-valid artifact with wrong-typed fields and +asserting the parse degrades to the field default / a skipped row (no exception). + +**Resolution:** Resolved 2026-06-19 — added `DeploymentArtifactMalformedTypeTests` (string `Enabled`, numeric +`Name`/`DriverType`, numeric `UnsAreaId`) asserting `ParseDriverInstances` + `ParseComposition` degrade +gracefully instead of throwing. Co-resolved with Runtime-002. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs index 6116dce7..e803a157 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DeploymentArtifact.cs @@ -145,16 +145,27 @@ public static class DeploymentArtifact : all; } + /// Reads a string property, tolerating a wrong JSON type: a property that is present but + /// NOT a JSON string (number / bool / object / array) yields null rather than throwing + /// from GetString(). Keeps the artifact-decode lenient + /// — a wrong-typed field degrades to the field default / a skipped row, matching the documented + /// "malformed blobs return an empty list/composition" contract (Runtime-002). + private static string? ReadString(JsonElement el, string property) => + el.TryGetProperty(property, out var p) && p.ValueKind == JsonValueKind.String ? p.GetString() : null; + private static DriverInstanceSpec? TryReadSpec(JsonElement el) { var rowId = el.TryGetProperty("DriverInstanceRowId", out var rowEl) && rowEl.TryGetGuid(out var rid) ? rid : Guid.Empty; - var id = el.TryGetProperty("DriverInstanceId", out var idEl) ? idEl.GetString() : null; - var name = el.TryGetProperty("Name", out var nameEl) ? nameEl.GetString() : null; - var type = el.TryGetProperty("DriverType", out var typeEl) ? typeEl.GetString() : null; - var enabled = !el.TryGetProperty("Enabled", out var enEl) || enEl.GetBoolean(); - var config = el.TryGetProperty("DriverConfig", out var cfgEl) ? cfgEl.GetString() : null; - var clusterId = el.TryGetProperty("ClusterId", out var clEl) ? clEl.GetString() : null; + var id = ReadString(el, "DriverInstanceId"); + var name = ReadString(el, "Name"); + var type = ReadString(el, "DriverType"); + // A non-bool Enabled token degrades to the field default (true = "desired here"), never throws. + var enabled = !el.TryGetProperty("Enabled", out var enEl) + || enEl.ValueKind is not (JsonValueKind.True or JsonValueKind.False) + || enEl.GetBoolean(); + var config = ReadString(el, "DriverConfig"); + var clusterId = ReadString(el, "ClusterId"); if (string.IsNullOrWhiteSpace(id) || string.IsNullOrWhiteSpace(type)) return null; @@ -781,48 +792,48 @@ public static class DeploymentArtifact private static UnsAreaProjection? ReadAreaProjection(JsonElement el) { - var id = el.TryGetProperty("UnsAreaId", out var idEl) ? idEl.GetString() : null; - var name = el.TryGetProperty("Name", out var nameEl) ? nameEl.GetString() : null; + var id = ReadString(el, "UnsAreaId"); + var name = ReadString(el, "Name"); if (string.IsNullOrWhiteSpace(id)) return null; return new UnsAreaProjection(id!, name ?? id!); } private static UnsLineProjection? ReadLineProjection(JsonElement el) { - var id = el.TryGetProperty("UnsLineId", out var idEl) ? idEl.GetString() : null; - var areaId = el.TryGetProperty("UnsAreaId", out var areaEl) ? areaEl.GetString() : null; - var name = el.TryGetProperty("Name", out var nameEl) ? nameEl.GetString() : null; + var id = ReadString(el, "UnsLineId"); + var areaId = ReadString(el, "UnsAreaId"); + var name = ReadString(el, "Name"); if (string.IsNullOrWhiteSpace(id) || string.IsNullOrWhiteSpace(areaId)) return null; return new UnsLineProjection(id!, areaId!, name ?? id!); } private static EquipmentNode? ReadEquipmentNode(JsonElement el) { - var id = el.TryGetProperty("EquipmentId", out var idEl) ? idEl.GetString() : null; + var id = ReadString(el, "EquipmentId"); // DisplayName = the UNS level-5 Name segment (friendly browse name, matching UnsArea/UnsLine // + the live rebuild's source of truth) — NOT the colloquial MachineCode. NodeId stays the // logical EquipmentId. - var displayName = el.TryGetProperty("Name", out var nameEl) ? nameEl.GetString() : null; - var lineId = el.TryGetProperty("UnsLineId", out var lineEl) ? lineEl.GetString() : null; + var displayName = ReadString(el, "Name"); + var lineId = ReadString(el, "UnsLineId"); if (string.IsNullOrWhiteSpace(id)) return null; return new EquipmentNode(id!, displayName ?? id!, lineId ?? string.Empty); } private static DriverInstancePlan? ReadDriverPlan(JsonElement el) { - var id = el.TryGetProperty("DriverInstanceId", out var idEl) ? idEl.GetString() : null; - var type = el.TryGetProperty("DriverType", out var typeEl) ? typeEl.GetString() : null; - var config = el.TryGetProperty("DriverConfig", out var cfgEl) ? cfgEl.GetString() : null; + var id = ReadString(el, "DriverInstanceId"); + var type = ReadString(el, "DriverType"); + var config = ReadString(el, "DriverConfig"); if (string.IsNullOrWhiteSpace(id) || string.IsNullOrWhiteSpace(type)) return null; return new DriverInstancePlan(id!, type!, config ?? "{}"); } private static ScriptedAlarmPlan? ReadAlarmPlan(JsonElement el) { - var id = el.TryGetProperty("ScriptedAlarmId", out var idEl) ? idEl.GetString() : null; - var equipmentId = el.TryGetProperty("EquipmentId", out var eqEl) ? eqEl.GetString() : null; - var script = el.TryGetProperty("PredicateScriptId", out var scEl) ? scEl.GetString() : null; - var template = el.TryGetProperty("MessageTemplate", out var tmEl) ? tmEl.GetString() : null; + var id = ReadString(el, "ScriptedAlarmId"); + var equipmentId = ReadString(el, "EquipmentId"); + var script = ReadString(el, "PredicateScriptId"); + var template = ReadString(el, "MessageTemplate"); if (string.IsNullOrWhiteSpace(id)) return null; return new ScriptedAlarmPlan(id!, equipmentId ?? string.Empty, script ?? string.Empty, template ?? string.Empty); } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DeploymentArtifactMalformedTypeTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DeploymentArtifactMalformedTypeTests.cs new file mode 100644 index 00000000..0a9cef9f --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/DeploymentArtifactMalformedTypeTests.cs @@ -0,0 +1,127 @@ +using System; +using System.Linq; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// Regression coverage for Runtime-002: the artifact-decode is documented as lenient — "empty / +/// malformed blobs return an empty list / empty composition". A blob that is syntactically valid JSON +/// but carries a field of the WRONG JSON type (e.g. a string Enabled, a numeric Name) must NOT throw +/// out of the parse (the old code called GetString()/GetBoolean() +/// unguarded). It must degrade to the field default / a skipped row instead. +/// +public sealed class DeploymentArtifactMalformedTypeTests +{ + private static byte[] BlobOf(object snapshot) => + System.Text.Json.JsonSerializer.SerializeToUtf8Bytes(snapshot); + + /// A string-typed Enabled ("true") must not throw; the row should still decode (default Enabled=true). + [Fact] + public void ParseDriverInstances_string_typed_Enabled_does_not_throw() + { + var blob = BlobOf(new + { + DriverInstances = new[] + { + new + { + DriverInstanceRowId = Guid.NewGuid(), + DriverInstanceId = "d1", + Name = "drv-1", + DriverType = "Modbus", + Enabled = "true", // WRONG TYPE: string, not bool + DriverConfig = "{}", + }, + }, + }); + + var specs = Should.NotThrow(() => DeploymentArtifact.ParseDriverInstances(blob)); + specs.Count.ShouldBe(1); + // A non-bool Enabled degrades to the field default (Enabled = true / "desired here"). + specs[0].Enabled.ShouldBeTrue(); + specs[0].DriverInstanceId.ShouldBe("d1"); + } + + /// A numeric-typed Name / DriverType must not throw; a row with an unusable required field + /// (DriverType missing/wrong) is skipped, a row whose only wrong field is the optional Name keeps decoding. + [Fact] + public void ParseDriverInstances_numeric_typed_Name_does_not_throw() + { + var blob = BlobOf(new + { + DriverInstances = new[] + { + new + { + DriverInstanceRowId = (object)Guid.NewGuid(), + DriverInstanceId = (object)"d1", + Name = (object)5, // WRONG TYPE: number, not string + DriverType = (object)"Modbus", + Enabled = (object)true, + DriverConfig = (object)"{}", + }, + }, + }); + + var specs = Should.NotThrow(() => DeploymentArtifact.ParseDriverInstances(blob)); + specs.Count.ShouldBe(1); + // A non-string Name degrades to null → Name falls back to the DriverInstanceId. + specs[0].Name.ShouldBe("d1"); + } + + /// The composition decode must also tolerate a wrong-typed UNS field (numeric UnsAreaId) + /// without throwing — it degrades to skipping that row. + [Fact] + public void ParseComposition_numeric_typed_UnsAreaId_does_not_throw() + { + var blob = BlobOf(new + { + UnsAreas = new object[] + { + new { UnsAreaId = 42, Name = "Area-numeric" }, // WRONG TYPE: numeric id → skipped + new { UnsAreaId = "area-ok", Name = "Area-OK" }, + }, + }); + + var composition = Should.NotThrow(() => DeploymentArtifact.ParseComposition(blob)); + // The well-typed area survives; the numeric-id one is skipped (id resolves to null → dropped). + composition.UnsAreas.Select(a => a.UnsAreaId).ShouldBe(new[] { "area-ok" }); + } + + /// A driver row whose DriverType is the wrong type is skipped (required field unusable), + /// not a parse failure. + [Fact] + public void ParseDriverInstances_numeric_typed_DriverType_skips_row_without_throwing() + { + var blob = BlobOf(new + { + DriverInstances = new object[] + { + new + { + DriverInstanceRowId = (object)Guid.NewGuid(), + DriverInstanceId = (object)"bad", + Name = (object)"drv-bad", + DriverType = (object)7, // WRONG TYPE: numeric → unusable required field + Enabled = (object)true, + DriverConfig = (object)"{}", + }, + new + { + DriverInstanceRowId = (object)Guid.NewGuid(), + DriverInstanceId = (object)"good", + Name = (object)"drv-good", + DriverType = (object)"Modbus", + Enabled = (object)true, + DriverConfig = (object)"{}", + }, + }, + }); + + var specs = Should.NotThrow(() => DeploymentArtifact.ParseDriverInstances(blob)); + specs.Select(s => s.DriverInstanceId).ShouldBe(new[] { "good" }); + } +}