From 3bb2031d1d7ca9d46a85112047e782108dc0e719 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Tue, 16 Jun 2026 22:31:15 -0400
Subject: [PATCH] fix(opcua): array equipment-tag nodes are read-only (array
 writes out of scope, review M-1)

---
 .../Phase7Applier.cs                          |  6 +-
 .../Phase7ApplierTests.cs                     | 60 +++++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Phase7Applier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Phase7Applier.cs
index fe6d6e7e..038b3a14 100644
--- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Phase7Applier.cs
+++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Phase7Applier.cs
@@ -211,7 +211,11 @@ public sealed class Phase7Applier
                 string? historianTagname = tag.IsHistorized
                     ? (string.IsNullOrWhiteSpace(tag.HistorianTagname) ? tag.FullName : tag.HistorianTagname)
                     : null;
-                SafeEnsureVariable(nodeId, parent, tag.Name, tag.DataType, tag.Writable, historianTagname, tag.IsArray, tag.ArrayLength);
+                // Array writes are out of scope (Phase 4c read-only surface): force array tags read-only
+                // even if authored ReadWrite, so a client write cannot reach the driver write path which
+                // does not handle arrays (e.g. S7 BoxValueForWrite would crash).
+                var writable = tag.Writable && !tag.IsArray;
+                SafeEnsureVariable(nodeId, parent, tag.Name, tag.DataType, writable, historianTagname, tag.IsArray, tag.ArrayLength);
             }
         }
 
diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/Phase7ApplierTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/Phase7ApplierTests.cs
index bebfef41..bc9bd25c 100644
--- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/Phase7ApplierTests.cs
+++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/Phase7ApplierTests.cs
@@ -378,6 +378,66 @@ public sealed class Phase7ApplierTests
         call.ArrayLength.ShouldBeNull();
     }
 
+    /// <summary>Review M-1 — an array equipment tag authored with <c>Writable: true</c> must be
+    /// materialised as READ-ONLY (<c>writable == false</c>) because array writes are out of scope
+    /// (Phase 4c read-only surface). The driver write path does not handle arrays and would crash
+    /// (e.g. S7 BoxValueForWrite). Guards against a future refactor that accidentally enables the
+    /// writable path for arrays.</summary>
+    [Fact]
+    public void MaterialiseEquipmentTags_array_writable_true_is_forced_read_only()
+    {
+        var sink = new RecordingSink();
+        var applier = new Phase7Applier(sink, NullLogger<Phase7Applier>.Instance);
+
+        var composition = new Phase7CompositionResult(
+            Array.Empty<EquipmentNode>(), Array.Empty<DriverInstancePlan>(), Array.Empty<ScriptedAlarmPlan>())
+        {
+            EquipmentTags = new[]
+            {
+                // Authored ReadWrite AND IsArray — the applier must clamp to read-only.
+                new EquipmentTagPlan("tag-arr-rw", "eq-1", "drv", FolderPath: "", Name: "Buffer", DataType: "Int16",
+                    FullName: "40001", Writable: true, Alarm: null, IsArray: true, ArrayLength: 8u),
+            },
+        };
+
+        applier.MaterialiseEquipmentTags(composition);
+
+        // writable must be false (array writes out of scope), isArray must be true (forwarded verbatim).
+        var varCall = sink.VariableCalls.ShouldHaveSingleItem();
+        varCall.Writable.ShouldBeFalse();   // clamped to read-only despite Writable: true
+        var arrCall = sink.ArrayCalls.ShouldHaveSingleItem();
+        arrCall.IsArray.ShouldBeTrue();
+        arrCall.ArrayLength.ShouldBe(8u);
+    }
+
+    /// <summary>Review M-1 regression — a scalar tag authored with <c>Writable: true</c> must still
+    /// be materialised as read/write (<c>writable == true</c>). The array-clamp must NOT affect
+    /// scalar tags.</summary>
+    [Fact]
+    public void MaterialiseEquipmentTags_scalar_writable_true_stays_writable()
+    {
+        var sink = new RecordingSink();
+        var applier = new Phase7Applier(sink, NullLogger<Phase7Applier>.Instance);
+
+        var composition = new Phase7CompositionResult(
+            Array.Empty<EquipmentNode>(), Array.Empty<DriverInstancePlan>(), Array.Empty<ScriptedAlarmPlan>())
+        {
+            EquipmentTags = new[]
+            {
+                // Authored ReadWrite, scalar — must pass through writable: true unchanged.
+                new EquipmentTagPlan("tag-scalar-rw", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float",
+                    FullName: "40002", Writable: true, Alarm: null, IsArray: false, ArrayLength: null),
+            },
+        };
+
+        applier.MaterialiseEquipmentTags(composition);
+
+        var varCall = sink.VariableCalls.ShouldHaveSingleItem();
+        varCall.Writable.ShouldBeTrue();   // scalar: writable unchanged
+        var arrCall = sink.ArrayCalls.ShouldHaveSingleItem();
+        arrCall.IsArray.ShouldBeFalse();
+    }
+
     /// <summary>Verifies MaterialiseEquipmentVirtualTags creates one Variable per VirtualTag directly
     /// under its existing equipment folder, with a folder-scoped NodeId (EquipmentId/Name — NOT the
     /// VirtualTagId or Expression), parent == EquipmentId, displayName == Name, and does NOT re-create