Merge b626-livealarm: client alarm filter browse paths + exact Part 9 condition severity

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 16:20:51 -04:00
parent cc7cd2db11 aba3aa7a8d
commit cd03b1b51d
9 changed files with 171 additions and 44 deletions
@@ -248,6 +248,12 @@ otopcua-cli alarms -u opc.tcp://localhost:4840/OtOpcUa \
 | `-i` / `--interval` | Publishing interval in milliseconds (default: 1000) |
 | `--refresh` | Request a `ConditionRefresh` after subscribing to get current retained alarm states |

+The active/acknowledged columns come from the standard `ActiveState/Id`, `AckedState/Id` and
+`EnabledState/Id` event fields. Before the 2026-10 fix the client's filter selected them with a single
+QualifiedName named `ActiveState/Id` (a `/`-joined string passed to `EventFilter.AddSelectClause` is **not**
+split into a browse path), which matched nothing — so every event printed inactive and unacknowledged
+whatever the condition's real state. A direct Read of the condition was always correct.
+
 ### ack

 Acknowledges an active alarm condition via the OPC UA Part 9 `Acknowledge` method. The server gates this on the **`AlarmAck`** role (fail-closed — a session whose API-key principal holds no `AlarmAck`-conferring `NodeAcl` grant gets `BadUserAccessDenied`; the data plane does not use LDAP; see [ScriptedAlarms.md](ScriptedAlarms.md) §"AlarmAck gate"). The `--event-id` is the hex-encoded `EventId` taken from a prior `alarms` subscription notification for the same condition.
@@ -274,9 +274,10 @@ overwrites an already-authored alarm section.
 #### Severity mapping

 The authored `severity` (1–1000) seeds the OPC UA condition node at materialisation
-time. Every native transition maps the driver's `AlarmSeverity` to one of four fixed
-buckets via `NativeAlarmProjector.MapSeverity`, overriding the authored `severity` seed
-from the first transition onward:
+time. Every native transition replaces it from the first transition onward. A source with a
+finer scale supplies the exact Part 9 severity (`AlarmEventArgs.OpcUaSeverity`, e.g. MTConnect
+Fault = 800, #626); otherwise `NativeAlarmProjector.MapSeverity` maps the driver's
+`AlarmSeverity` to one of four fixed values:

 | `AlarmSeverity` enum | Projected value |
 |---|---|
@@ -286,20 +287,12 @@ from the first transition onward:
 | `Critical` | 900 |
 | *(default/unspecified)* | 500 |

-That projected integer is then mapped to the OPC UA SDK `EventSeverity` enum
-by `OtOpcUaNodeManager.MapSeverity` on each `AlarmStateUpdate` write:
-
-| Projected value range | `EventSeverity` |
-|---|---|
-| < 200 | `Low` |
-| 200 – 399 | `MediumLow` |
-| 400 – 599 | `Medium` |
-| 600 – 799 | `MediumHigh` |
-| ≥ 800 | `High` |
-
-Consequently the four authored buckets land as: Low→`MediumLow`, Medium→`Medium`,
-High→`MediumHigh`, Critical→`High`. The authored `severity` field is overridden by
-live driver events on every transition.
+The condition node carries that projected integer **verbatim** (clamped to 1–1000), on a direct
+Read and on every event. It used to be re-bucketed onto the SDK's five `EventSeverity` points
+(< 200 → 100, < 400 → 300, < 600 → 500, < 800 → 700, else 900) by the node manager — so an
+MTConnect Fault read and evented as 900, a `Low` native alarm as 300, and a scripted alarm the
+engine graded 1000 published 900 while its `/alerts` row said 1000. That re-bucketing is gone
+(the `EventSeverity` enum merely names points on the 1–1000 scale; any value is valid).

 ### Runtime flow

@@ -867,7 +867,7 @@ public sealed class OpcUaClientService : IOpcUaClientService
        catch { return false; }
    }

-    private static EventFilter CreateAlarmEventFilter()
+    internal static EventFilter CreateAlarmEventFilter()
    {
        var filter = new EventFilter();
        // 0: EventId
@@ -886,12 +886,18 @@ public sealed class OpcUaClientService : IOpcUaClientService
        filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.ConditionName);
        // 7: Retain
        filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.Retain);
+        // 8-10: the two-state /Id booleans. Each browse path is TWO QualifiedNames. Passing "AckedState/Id" as a
+        // string to AddSelectClause does not split it — it selects one element NAMED "AckedState/Id", which no
+        // event carries, so all three arrived null and every alarm read inactive + unacknowledged.
        // 8: AckedState/Id
-        filter.AddSelectClause(ObjectTypeIds.AcknowledgeableConditionType, "AckedState/Id");
+        filter.SelectClauses.Add(new SimpleAttributeOperand(
+            ObjectTypeIds.AcknowledgeableConditionType, [BrowseNames.AckedState, BrowseNames.Id]));
        // 9: ActiveState/Id
-        filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "ActiveState/Id");
+        filter.SelectClauses.Add(new SimpleAttributeOperand(
+            ObjectTypeIds.AlarmConditionType, [BrowseNames.ActiveState, BrowseNames.Id]));
        // 10: EnabledState/Id
-        filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "EnabledState/Id");
+        filter.SelectClauses.Add(new SimpleAttributeOperand(
+            ObjectTypeIds.ConditionType, [BrowseNames.EnabledState, BrowseNames.Id]));
        // 11: SuppressedOrShelved
        filter.AddSelectClause(ObjectTypeIds.AlarmConditionType, "SuppressedOrShelved");
        // 12: SourceNode (ConditionId for acknowledgment)
@@ -888,7 +888,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
                        oneShot: state.Shelving == AlarmShelvingKind.OneShot,
                        shelvingTime: state.Shelving == AlarmShelvingKind.Timed ? RemainingShelveMs(state.UnshelveAtUtc) : 0);
                }
-                condition.SetSeverity(SystemContext, MapSeverity(state.Severity));
+                condition.SetSeverity(SystemContext, ToPart9Severity(state.Severity));
                condition.Message.Value = new LocalizedText(state.Message);

                // #477 — project the source-data quality. A Good→Bad bucket change is a genuine condition
@@ -1075,9 +1075,9 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    ///     condition event AND that an <see cref="AlarmConditionSnapshot"/> can change. As a record, two
    ///     instances compare by value, so <see cref="ShouldFireConditionEvent"/> is a plain inequality.
    ///     <para>
-    ///         <b>Severity</b> is stored as the MAPPED <see cref="EventSeverity"/> bucket (a
-    ///         <see cref="ushort"/>) — the same value the node holds after <c>SetSeverity</c> — so two
-    ///         raw severities that fall in the same bucket are correctly treated as "no change". The
+    ///         <b>Severity</b> is stored as the value the node holds after <c>SetSeverity</c> — the exact
+    ///         Part 9 severity clamped to 1..1000 (<see cref="ToPart9Severity"/>; it used to be one of five
+    ///         <see cref="EventSeverity"/> buckets, see there) — so a severity change fires an event. The
    ///         <b>Shelving</b> kind is read back from / mapped to the SDK's shelving state machine so the
    ///         live node and the snapshot compare on the same 3-way (Unshelved/OneShot/Timed) axis.
    ///     </para>
@@ -1134,8 +1134,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2

    /// <summary>Read the gate-relevant slice off the LIVE condition node. Mandatory children
    /// (Active/Acked/Enabled) are always present; Confirmed/Shelving are optional and null-guarded
-    /// (a leaner child set ⇒ treat as the unset default). Severity is read as the already-mapped
-    /// <see cref="EventSeverity"/> bucket the node stores, and shelving is mapped from the shelving
+    /// (a leaner child set ⇒ treat as the unset default). Severity is read as the already-clamped
+    /// value the node stores, and shelving is mapped from the shelving
    /// state machine's CurrentState so it lines up with <see cref="ToConditionDelta"/>.</summary>
    private static AlarmConditionDelta ReadConditionDelta(AlarmConditionState condition) => new(
        Active: condition.ActiveState?.Id?.Value ?? false,
@@ -1150,8 +1150,8 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        Quality: condition.Quality?.Value ?? StatusCodes.Good);

    /// <summary>Build the gate-relevant slice from the incoming snapshot, normalising the two fields that
-    /// the node stores in a derived form: Severity is run through <see cref="MapSeverity"/> so it matches
-    /// the bucket the node holds (the projection calls <c>SetSeverity(MapSeverity(...))</c>), and an
+    /// the node stores in a derived form: Severity is run through <see cref="ToPart9Severity"/> so it matches
+    /// the value the node holds (the projection calls <c>SetSeverity(ToPart9Severity(...))</c>), and an
    /// optional Confirmed/Shelving that the node can't actually hold (missing child) is folded to the
    /// node's read-back default so it never spuriously registers as a delta.</summary>
    private static AlarmConditionDelta ToConditionDelta(AlarmConditionSnapshot state, AlarmConditionState condition) => new(
@@ -1164,7 +1164,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        // Likewise for shelving: without a ShelvingState child the projection can't apply, so fold to the
        // node's read-back default (Unshelved).
        Shelving: condition.ShelvingState is not null ? state.Shelving : AlarmShelvingKind.Unshelved,
-        MappedSeverity: (ushort)MapSeverity(state.Severity),
+        MappedSeverity: (ushort)ToPart9Severity(state.Severity),
        Message: state.Message ?? string.Empty,
        // If the node has no Quality child, WriteAlarmCondition's projection is a no-op there; fold to the
        // node's read-back default (Good) so a snapshot Quality can't register a spurious delta.
@@ -1200,7 +1200,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    /// <param name="equipmentNodeId">The equipment folder node id the condition parents under (null/unknown ⇒ root).</param>
    /// <param name="displayName">Human-readable condition name (BrowseName / DisplayName / Message / ConditionName).</param>
    /// <param name="alarmType">Domain alarm type — maps to the SDK condition subtype (see remarks).</param>
-    /// <param name="severity">Domain severity (treated as an OPC UA 1..1000 severity); mapped to <see cref="EventSeverity"/>.</param>
+    /// <param name="severity">Domain severity (an OPC UA 1..1000 severity); written verbatim, clamped to 1..1000.</param>
    /// <param name="isNative">True when the condition is a NATIVE (driver-fed, e.g. Galaxy) alarm rather than a scripted one.</param>
    /// <remarks>
    ///     <para><b>AlarmType → SDK subtype mapping.</b> Script-driven alarms have no OPC limit /
@@ -1327,7 +1327,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            // refused and Acknowledge re-asserts Confirmed — so start it Confirmed, the value NativeAlarmProjector
            // supplies on every transition, rather than the SDK's default false until the first projection.
            if (isNative && alarm.ConfirmedState is not null) alarm.SetConfirmedState(SystemContext, true);
-            alarm.SetSeverity(SystemContext, MapSeverity(severity));
+            alarm.SetSeverity(SystemContext, ToPart9Severity(severity));
            // Gitea #587 (found by its TimedShelve test): Create() leaves MaxTimeShelved at 0, and the SDK's
            // TimedShelve handler rejects any ShelvingTime above it — so every client TimedShelve failed with
            // BadShelvingTimeOutOfRange. The bound becomes the longest shelve the SDK's own shelving timer can
@@ -2618,16 +2618,20 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        folder.ClearChangeMasks(SystemContext, includeChildren: false);
    }

-    /// <summary>Map an integer domain severity (treated as the OPC UA 1..1000 scale) onto the
-    /// <see cref="EventSeverity"/> enum buckets the SDK's <c>SetSeverity</c> expects.</summary>
-    private static EventSeverity MapSeverity(int severity) => severity switch
-    {
-        < 200 => EventSeverity.Low,
-        < 400 => EventSeverity.MediumLow,
-        < 600 => EventSeverity.Medium,
-        < 800 => EventSeverity.MediumHigh,
-        _ => EventSeverity.High,
-    };
+    /// <summary>
+    ///     The Part 9 <c>Severity</c> (1..1000) a condition carries for a domain severity, which is already on
+    ///     that scale: the value itself, clamped. <c>SetSeverity</c> takes an <see cref="EventSeverity"/>, but
+    ///     that enum merely names five points on the 1..1000 scale — any value in range is a valid Severity.
+    /// </summary>
+    /// <remarks>
+    ///     This used to re-bucket onto those five points (<c>&lt; 800</c> ⇒ 700, else 900), so the condition
+    ///     node was the one seam that disagreed with every other: an MTConnect Fault (#626's exact 800) read and
+    ///     evented as 900, a scripted alarm the engine graded 1000 published 900 on the node while its
+    ///     <c>/alerts</c> row and historian event said 1000, and a bucket-only native driver's Low (200) became 300.
+    /// </remarks>
+    /// <param name="severity">The domain severity.</param>
+    /// <returns>The severity the node carries.</returns>
+    private static EventSeverity ToPart9Severity(int severity) => (EventSeverity)Math.Clamp(severity, 1, 1000);

    /// <summary>
    ///     Ensure a folder node exists at <paramref name="folderNodeId"/> with the given display
@@ -0,0 +1,38 @@
+using Opc.Ua;
+using Shouldly;
+using Xunit;
+
+namespace ZB.MOM.WW.OtOpcUa.Client.Shared.Tests;
+
+/// <summary>
+///     Pins the browse paths of the alarm subscription's select clauses. <c>EventFilter.AddSelectClause(type,
+///     "ActiveState/Id")</c> does NOT split the string — it yields ONE QualifiedName named <c>ActiveState/Id</c>,
+///     which matches nothing — so the client reported every alarm inactive and unacknowledged. The over-the-wire
+///     proof is <c>ClientAlarmMethodTests.The_client_alarm_subscription_reports_active_acked_and_exact_severity</c>.
+/// </summary>
+public sealed class AlarmEventFilterTests
+{
+    /// <summary>No select clause carries a QualifiedName with a path separator in it.</summary>
+    [Fact]
+    public void No_browse_path_element_contains_a_separator()
+    {
+        var filter = OpcUaClientService.CreateAlarmEventFilter();
+
+        filter.SelectClauses.SelectMany(c => c.BrowsePath).ShouldAllBe(q => !q.Name.Contains('/'));
+    }
+
+    /// <summary>The two-state ids are selected by their two-element browse paths, at the indices the parser reads.</summary>
+    [Fact]
+    public void Two_state_ids_are_two_element_browse_paths()
+    {
+        var clauses = OpcUaClientService.CreateAlarmEventFilter().SelectClauses;
+
+        string Path(int i) => string.Join("/", clauses[i].BrowsePath.Select(q => q.Name));
+        Path(8).ShouldBe("AckedState/Id");
+        clauses[8].BrowsePath.Count.ShouldBe(2);
+        Path(9).ShouldBe("ActiveState/Id");
+        clauses[9].BrowsePath.Count.ShouldBe(2);
+        Path(10).ShouldBe("EnabledState/Id");
+        clauses[10].BrowsePath.Count.ShouldBe(2);
+    }
+}
@@ -3,6 +3,7 @@ using Shouldly;
 using Xunit;
 using ZB.MOM.WW.OtOpcUa.Client.Shared;
 using ZB.MOM.WW.OtOpcUa.Client.Shared.Models;
+using ZB.MOM.WW.OtOpcUa.Commons.OpcUa;
 using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
 using static ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests.ServerEventRig;

@@ -135,6 +136,70 @@ public sealed class ClientAlarmMethodTests
        finally { await rig.DisposeAsync(); }
    }

+    /// <summary>
+    ///     The client's alarm SUBSCRIPTION (Client.CLI <c>alarms</c>) reads the condition's two-state fields off a
+    ///     real event. Its filter used to pass <c>"ActiveState/Id"</c> to <c>EventFilter.AddSelectClause</c>, which
+    ///     does NOT split the string: the operand's browse path was ONE QualifiedName named <c>ActiveState/Id</c>,
+    ///     which matches nothing, so every event arrived with Active/Acked/Enabled null and the client reported
+    ///     them all inactive and unacknowledged — while a direct Read of the same condition said Active. The
+    ///     fake-adapter unit tests feed field values straight in and so never exercise the filter.
+    /// </summary>
+    /// <param name="native">Drive the native (raw device folder) or the scripted (equipment folder) condition.</param>
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public async Task The_client_alarm_subscription_reports_active_acked_and_exact_severity(bool native)
+    {
+        var ct = TestContext.Current.CancellationToken;
+        var rig = await StartAsync($"ClientAlarmSubscribe{native}", encrypted: false, ct);
+        try
+        {
+            rig.Enforce(false, [Key()], ClusterGrant(KeyPrincipal, "acl-key", All));
+            if (native) rig.MaterialiseNative(); else rig.MaterialiseScripted();
+
+            using var client = await ConnectClientAsync(rig, ct);
+            using var observer = await rig.ConnectAsync(anonymous: false, ct);
+            var source = native
+                ? new NodeId(RawDeviceFolder, (ushort)observer.NamespaceUris.GetIndex(V3NodeIds.RawNamespaceUri))
+                : UnsNodeId(observer, EqA);
+
+            var seen = new List<AlarmEventArgs>();
+            client.AlarmEvent += (_, e) => { lock (seen) seen.Add(e); };
+            await client.SubscribeAlarmsAsync(source, 100, ct);
+
+            AlarmEventArgs? Latest(string message)
+            {
+                lock (seen) return seen.LastOrDefault(e => e.Message == message);
+            }
+
+            // 800 is not one of the SDK's five EventSeverity values (900 is): the node must carry it verbatim.
+            rig.Project(native, new AlarmConditionSnapshot(Active: true, Acknowledged: false, Confirmed: true,
+                Enabled: true, AlarmShelvingKind.Unshelved, 800, "raised"));
+            await RecordedEvents.WaitUntilAsync(() => Latest("raised") is not null);
+            var raised = Latest("raised")!;
+            raised.ActiveState.ShouldBeTrue("the condition is active");
+            raised.AckedState.ShouldBeFalse();
+            raised.Severity.ShouldBe((ushort)800);
+            raised.Retain.ShouldBeTrue();
+
+            rig.Project(native, new AlarmConditionSnapshot(Active: true, Acknowledged: true, Confirmed: true,
+                Enabled: true, AlarmShelvingKind.Unshelved, 800, "acked"));
+            await RecordedEvents.WaitUntilAsync(() => Latest("acked") is not null);
+            Latest("acked")!.AckedState.ShouldBeTrue("the positive control for the Acked field");
+            Latest("acked")!.ActiveState.ShouldBeTrue();
+
+            rig.Project(native, new AlarmConditionSnapshot(Active: false, Acknowledged: true, Confirmed: true,
+                Enabled: true, AlarmShelvingKind.Unshelved, 800, "cleared"));
+            await RecordedEvents.WaitUntilAsync(() => Latest("cleared") is not null);
+            var cleared = Latest("cleared")!;
+            cleared.ActiveState.ShouldBeFalse();
+            cleared.Retain.ShouldBeFalse("an inactive, acknowledged condition is not retained");
+
+            await client.UnsubscribeAlarmsAsync(ct);
+        }
+        finally { await rig.DisposeAsync(); }
+    }
+
    private static async Task<OpcUaClientService> ConnectClientAsync(ServerEventRig rig, CancellationToken ct)
    {
        var client = new OpcUaClientService();
@@ -78,6 +78,9 @@ public sealed class MTConnectConditionAlarmWireTests

            var fault = events.ConditionEvents(folderItem, FaultMessage).ShouldHaveSingleItem();
            fault[RecordedEvents.AckedField].Value.ShouldBe(false, "a fresh fault is unacknowledged");
+            fault[RecordedEvents.ActiveField].Value.ShouldBe(true, "a fault is active");
+            fault[RecordedEvents.SeverityField].Value.ShouldBe((ushort)800, "Fault is the exact Part 9 severity 800, not the 900 bucket");
+            fault[RecordedEvents.RetainField].Value.ShouldBe(true);

            // An operator acknowledges it (applied server-side, as the Part 9 method does), then a deploy clears
            // the host's projector prior state.
@@ -96,10 +99,15 @@ public sealed class MTConnectConditionAlarmWireTests
            foreach (var blip in events.ConditionEvents(folderItem, FaultMessage).Skip(2))
            {
                blip[RecordedEvents.AckedField].Value.ShouldBe(true, "an Unavailable blip must not reset the ack");
+                blip[RecordedEvents.ActiveField].Value.ShouldBe(true, "an Unavailable blip must not clear the fault");
            }

            agent.Push(Condition("Normal", null, null, 111));
            await RecordedEvents.WaitUntilAsync(() => events.Has(folderItem, "SYSTEM Normal"));
+            var normal = events.ConditionEvents(folderItem, "SYSTEM Normal").ShouldHaveSingleItem();
+            normal[RecordedEvents.ActiveField].Value.ShouldBe(false, "Normal clears the condition");
+            normal[RecordedEvents.RetainField].Value.ShouldBe(false, "inactive + acknowledged is no longer retained");
+            normal[RecordedEvents.SeverityField].Value.ShouldBe((ushort)800, "a clear keeps the severity of the alarm it clears");

            lock (routed) routed.ShouldAllBe(id => id == RawAlarmPath, "ConditionId must be the alarm tag's RawPath");

@@ -315,6 +315,9 @@ internal sealed class RecordedEvents
    public const int ChangesField = 8;
    public const int SourceNodeField = 9;
    public const int UnshelveTimeField = 10;
+    public const int ActiveField = 11;
+    public const int SeverityField = 12;
+    public const int RetainField = 13;

    private readonly object _gate = new();
    private readonly List<(uint Handle, VariantCollection Fields)> _events = [];
@@ -374,6 +377,10 @@ internal sealed class RecordedEvents
        filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.SourceNode);
        filter.SelectClauses.Add(new SimpleAttributeOperand(
            ObjectTypeIds.AlarmConditionType, [BrowseNames.ShelvingState, BrowseNames.UnshelveTime]));
+        filter.SelectClauses.Add(new SimpleAttributeOperand(
+            ObjectTypeIds.AlarmConditionType, [BrowseNames.ActiveState, BrowseNames.Id]));
+        filter.AddSelectClause(ObjectTypes.BaseEventType, BrowseNames.Severity);
+        filter.AddSelectClause(ObjectTypeIds.ConditionType, BrowseNames.Retain);

        var item = new MonitoredItem(subscription.DefaultItem)
        {
@@ -398,8 +398,8 @@ public sealed class SdkAddressSpaceSinkTests : IDisposable
        // SetShelvingState(shelved:true, oneShot:false) drives the shelving sub-state machine into a
        // shelved state; CurrentState is populated (TimedShelved).
        condition.ShelvingState!.CurrentState.Id.Value.ShouldNotBeNull();
-        // Severity 850 → High bucket (>= 800) → EventSeverity.High (the SDK stamps the ushort value).
-        condition.Severity.Value.ShouldBe((ushort)EventSeverity.High);
+        // Severity 850 is carried verbatim — not re-bucketed onto EventSeverity.High (900).
+        condition.Severity.Value.ShouldBe((ushort)850);
        condition.Message.Value.Text.ShouldBe("Pressure above limit");
        condition.Retain.Value.ShouldBeTrue();                   // active || !acked ⇒ retain