fix(opcua): drop non-condition events from the alarm path

src/ros2_medkit_plugins/ros2_medkit_opcua/README.md

@@ -334,8 +334,13 @@ only when all of them match the observed event; declaration order breaks ties.
 `condition_name` / `source_node` / `event_type` are equality matches;
 `match_message` is a case-sensitive substring match on the event Message.
 An event matching no mapping uses the source-level `fault_code`; if that is also
-empty the event is ignored. A mapping-level `severity_override` / `message`
-overrides the source-level one; otherwise the source-level value is inherited.
+empty the event is ignored. Exception: a non-condition event (a plain BaseEvent
+/ system message whose `ConditionId` resolves to null - e.g. a SIMATIC "CPU not
+in RUN" arriving on a `ns=0;i=2253` catch-all) is dropped before mapping
+resolution runs, so it never falls through to the source-level `fault_code` and
+that catch-all code is never raised for it (see "System messages" under
+`auto_alarms` below). A mapping-level `severity_override` / `message` overrides
+the source-level one; otherwise the source-level value is inherited.
 
 The plugin auto-registers `acknowledge_fault` and `confirm_fault` operations
 on every entity that has at least one `event_alarms` entry. Invoke them with:
@@ -493,8 +498,12 @@ no node-map file required.
 EventNotifier as real alarms - a Siemens Server object also emits SIMATIC
 system messages (e.g. `"CPU not in RUN"`) over `i=2253`. Per OPC-UA Part 9
 §5.5.2.13 only a true Condition instance carries a ConditionId; a system
-message resolves it to `NodeId.Null`, and `auto_alarms` drops those events
-before deriving a fault. Use `exclude` for anything that still needs
+message resolves it to `NodeId.Null`. The bridge drops those events for every
+alarm source - explicit `event_alarms` (including a source-level `fault_code`
+catch-all) and `auto_alarms` alike - before any mapping resolution or
+auto-derivation, so they never surface as a fault. On an explicit source it also
+logs a one-time warning (a real alarm that genuinely lacks a ConditionId would
+otherwise be dropped silently). Use `exclude` for anything that still needs
 filtering by text (e.g. a server that does set a ConditionId on system
 events).
 

src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst

@@ -220,6 +220,8 @@ State machine
 
 Inputs from each event payload (positional ``EventFilter`` select clauses):
 
+- ``ConditionId`` - NodeId; null means the notification is not an A&C Condition
+  (a plain BaseEvent / system message), gated by rule 0 below before the rest
 - ``EnabledState.Id`` - bool
 - ``ShelvingState.CurrentState.Id`` - NodeId; non-Unshelved => suppressed
 - ``ActiveState.Id`` - bool
@@ -232,6 +234,10 @@ Decision order, first match wins:
 +-----+--------------------------------------+---------------------------------------+
 | #   | Condition                            | Outcome                               |
 +=====+======================================+=======================================+
+| 0   | ``ConditionId == null``              | not a Condition: dropped for every    |
+|     | (non-condition event, e.g. a         | alarm source - explicit and auto -    |
+|     | system message)                      | before the state machine runs         |
++-----+--------------------------------------+---------------------------------------+
 | 1   | ``BranchId != null``                 | history-only (no SOVD update)         |
 +-----+--------------------------------------+---------------------------------------+
 | 2   | ``EnabledState == false``            | clear if was active, else no-op       |
@@ -254,6 +260,13 @@ Decision order, first match wins:
 |     | not ``require_confirm_for_clear``)   |                                       |
 +-----+--------------------------------------+---------------------------------------+
 
+Rule 0 is enforced in the poller's ``on_event`` (via ``is_condition_event``)
+before any state-machine input is built, and applies to every alarm source -
+explicit ``event_alarms`` (including a source-level ``fault_code`` catch-all on
+``ns=0;i=2253``) and zero-config ``auto_alarms`` alike. A drop on an explicit
+source is additionally logged once as an operator warning, since a real alarm
+that genuinely lacks a ConditionId would otherwise vanish without a trace.
+
 ``require_confirm_for_clear`` (default ``true``) gates rule 5b on both
 Acknowledge and Confirm. Some servers do not implement the optional ``Confirm``
 transition - Siemens S7-1500 supports ``Acknowledge`` / ``ConditionRefresh`` /

src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_poller.hpp

@@ -266,9 +266,11 @@ class OpcuaPoller {
   /// ConditionId SAO resolves to a non-null NodeId only for AlarmConditionType
   /// (and subtype) instances; a plain BaseEvent / SystemEvent notification -
   /// e.g. a Siemens Server-object "CPU not in RUN" system message delivered on
-  /// the same EventNotifier (i=2253) auto_alarms subscribes to - carries no
-  /// ConditionId and must not be auto-derived into a fault. Pure and static so
-  /// the system-message filter is unit-testable without a server.
+  /// the same EventNotifier (i=2253) an alarm source subscribes to - carries no
+  /// ConditionId. ``on_event`` drops such an event for EVERY alarm source,
+  /// explicit ``event_alarms`` and zero-config ``auto_alarms`` alike, before the
+  /// mapping/auto-derivation split, so it never becomes a fault. Pure and static
+  /// so the guard is unit-testable without a server.
   static bool is_condition_event(const opcua::NodeId & condition_id);
 
  private:
@@ -390,6 +392,10 @@ class OpcuaPoller {
   std::set<std::string> read_modeled_sources_;
   // Sources already warned about as read-fallback-unsupported (warn once each).
   std::set<std::string> read_unsupported_warned_sources_;
+  // Configured alarm sources already warned that a non-condition (null
+  // ConditionId) event was dropped despite their explicit fault_code/mappings
+  // (warn once each; see the guard in on_event).
+  std::set<std::string> noncondition_drop_warned_sources_;
 
   // ConditionRefresh bracketing state. open62541 sends the buffered historical
   // condition burst between RefreshStartEvent and RefreshEndEvent; we apply each

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp

@@ -752,6 +752,51 @@ void OpcuaPoller::on_event(const AlarmEventConfig & cfg, const std::vector<opcua
     }
   }
 
+  // Drop non-condition events for EVERY alarm source, explicit or auto, before
+  // the explicit/auto split below. The server-wide EventNotifier (ns=0;i=2253)
+  // also emits BaseEventType / AuditEventType housekeeping (e.g. "Session state
+  // changed to Created ...") that is not an A&C Condition and carries a null
+  // ConditionId (Part 9 §5.5.2.13: only a Condition resolves the ConditionId
+  // select clause). Such an event does NOT itself surface a fault: every
+  // alarm-state select clause resolves to null, so apply_condition_state feeds
+  // ActiveState=false from a fresh Suppressed entry into AlarmStateMachine,
+  // which returns ReportHealed - and on_event_alarm treats ReportHealed as a
+  // no-op. The harm is the latched null-ConditionId entry that reaching
+  // apply_condition_state leaves behind in conditions_ (keyed by the null
+  // NodeId string, pinned at the resolved fault_code - e.g. a catch-all
+  // event_alarms i=2253 -> PLC_ALARM), latched at Healed:
+  //   (a) should_clear_after_refresh counts Healed as active, and the null cid
+  //       is never replayed inside a ConditionRefresh burst, so the next
+  //       RefreshEnd emits a spurious ClearFault carrying that fault_code -
+  //       clearing a genuine alarm that shares the code;
+  //   (b) that null entry can win lookup_condition's linear (entity_id,
+  //       fault_code) scan, so a later Acknowledge / Confirm is misrouted to a
+  //       null ConditionId (call_method on runtime->condition_id).
+  // Dropping the event up front stops the entry from ever being created.
+  if (!is_condition_event(condition_id)) {
+    // On an EXPLICIT source (operator supplied a fault_code and/or mappings) a
+    // real alarm can legitimately arrive with a null ConditionId - an edge
+    // open62541 server built without UA_ENABLE_SUBSCRIPTIONS_ALARMS_CONDITIONS,
+    // or an AlarmConditionType fired via UA_Server_triggerEvent rather than
+    // createCondition. Dropping it with only the DEBUG line below would regress
+    // that path silently (pre-PR it still raised), so warn the operator once per
+    // source. The synthetic auto source carries no fault_code/mappings
+    // (effective_alarm_sources leaves them empty), so this predicate excludes
+    // it and the auto/synthetic path keeps only the DEBUG line.
+    if ((!cfg.fault_code.empty() || !cfg.mappings.empty()) &&
+        noncondition_drop_warned_sources_.insert(cfg.source_node_id_str).second) {
+      const std::string code_desc =
+          cfg.fault_code.empty() ? std::string("its mapped fault codes") : "fault_code '" + cfg.fault_code + "'";
+      warn_operator("OPC-UA non-condition event on configured alarm source '" + cfg.source_node_id_str + "' (" +
+                    code_desc +
+                    ") carries a null ConditionId and was dropped; if this source's alarms carry no ConditionId they "
+                    "will not surface as faults.");
+    }
+    RCLCPP_DEBUG_STREAM(opcua_poller_logger(), "on_event: non-condition event (null ConditionId, type="
+                                                   << event_type.toString() << ") - ignoring");
+    return;
+  }
+
   // A condition (re)reported inside a refresh burst is still active; remember it
   // so RefreshEnd does not clear it. Recorded before the field-count bail below
   // so even a partial-field replay counts as "seen".
@@ -863,20 +908,9 @@ void OpcuaPoller::on_event(const AlarmEventConfig & cfg, const std::vector<opcua
         return;
       }
     }
-    // System-message filter (validated on a real Siemens S7-1500): the
-    // Server object's EventNotifier (i=2253) also emits plain BaseEvent /
-    // SystemEvent notifications (e.g. "CPU not in RUN") that are NOT
-    // AlarmConditionType instances. Per Part 9 §5.5.2.13 only a real
-    // Condition carries a ConditionId; a system message resolves it to
-    // NodeId.Null, so reject those here rather than auto-deriving a
-    // fault for a message that never raises/clears.
-    if (!is_condition_event(condition_id)) {
-      RCLCPP_DEBUG_STREAM(opcua_poller_logger(),
-                          "on_event: dropping non-condition event (null ConditionId, likely a system "
-                          "message) message='"
-                              << message << "'");
-      return;
-    }
+    // Non-condition system messages (e.g. "CPU not in RUN") are already
+    // rejected for every source by the is_condition_event() guard at the top
+    // of on_event, so by here condition_id is guaranteed non-null.
     if (!NodeMap::auto_alarm_passes_filters(condition_name, source_name, message, auto_cfg.include_patterns,
                                             auto_cfg.exclude_patterns)) {
       return;

src/ros2_medkit_plugins/ros2_medkit_opcua/test/fixtures/test_alarm_server/test_alarm_server.cpp

@@ -269,6 +269,33 @@ UA_StatusCode handle_confirm(UA_Server * server, const Condition & c) {
   return UA_Server_triggerConditionEvent(server, c.node, c.source, nullptr);
 }
 
+// Fire a plain BaseEventType notification on the Server object (i=2253) - the
+// same EventNotifier a catch-all event_alarms / auto_alarms subscribes to. A
+// BaseEventType is not an AlarmConditionType, so it carries no ConditionId: the
+// gateway's ConditionId select clause resolves to NodeId.Null and the poller's
+// non-condition guard must drop it (it must never surface as a fault). This is
+// the regression fixture for that guard - the housekeeping notifications real
+// servers (e.g. a Siemens Server object) multiplex onto the alarm EventNotifier.
+UA_StatusCode handle_sysevent(UA_Server * server) {
+  UA_NodeId event_node = UA_NODEID_NULL;
+  UA_StatusCode rc = UA_Server_createEvent(server, UA_NODEID_NUMERIC(0, UA_NS0ID_BASEEVENTTYPE), &event_node);
+  if (rc != UA_STATUSCODE_GOOD) {
+    return rc;
+  }
+  UA_DateTime now = UA_DateTime_now();
+  UA_Server_writeObjectProperty_scalar(server, event_node, UA_QUALIFIEDNAME(0, const_cast<char *>("Time")), &now,
+                                       &UA_TYPES[UA_TYPES_DATETIME]);
+  UA_UInt16 severity = 500;
+  UA_Server_writeObjectProperty_scalar(server, event_node, UA_QUALIFIEDNAME(0, const_cast<char *>("Severity")),
+                                       &severity, &UA_TYPES[UA_TYPES_UINT16]);
+  UA_LocalizedText msg =
+      UA_LOCALIZEDTEXT(const_cast<char *>("en"), const_cast<char *>("Session state changed to Created"));
+  UA_Server_writeObjectProperty_scalar(server, event_node, UA_QUALIFIEDNAME(0, const_cast<char *>("Message")), &msg,
+                                       &UA_TYPES[UA_TYPES_LOCALIZEDTEXT]);
+  // originId = Server object (i=2253); delete the one-shot event node after.
+  return UA_Server_triggerEvent(server, event_node, UA_NODEID_NUMERIC(0, UA_NS0ID_SERVER), nullptr, UA_TRUE);
+}
+
 UA_StatusCode set_shelving(UA_Server * server, const Condition & c, bool shelved) {
   // ShelvingState is a ShelvedStateMachineType sub-object on the condition
   // (Part 9). open62541's experimental A&C does not implement the TimedShelve
@@ -612,6 +639,18 @@ void cli_loop(UA_Server * server, UA_UInt16 ns) {
       }
       continue;
     }
+    // ``sysevent`` fires a non-condition BaseEventType on the Server object
+    // (i=2253); it has no <name> and is not in g_conditions, so handle it
+    // before the condition lookup (like ``quit`` / ``set``).
+    if (cmd == "sysevent") {
+      UA_StatusCode rc = handle_sysevent(server);
+      if (rc == UA_STATUSCODE_GOOD) {
+        std::cout << "OK sysevent" << std::endl;
+      } else {
+        std::cout << "ERR sysevent:" << UA_StatusCode_name(rc) << std::endl;
+      }
+      continue;
+    }
     auto it = g_conditions.find(name);
     if (cmd != "quit" && it == g_conditions.end()) {
       std::cout << "ERR unknown_condition:" << name << std::endl;

src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_secured.test.py

@@ -69,6 +69,9 @@
 ROS_DOMAIN_ID = '228'
 
 ALARM_CODE = 'PLC_OVERPRESSURE'
+# Source-level catch-all code a non-condition event would be mis-mapped to if the
+# poller's ConditionId guard regressed.
+NONCOND_CODE = 'PLC_ALARM'
 
 
 def require_flag():
@@ -174,6 +177,32 @@ def write_node_map(path):
     )
 
 
+def write_catchall_node_map(path):
+    """
+    Node map with a single explicit event_alarms catch-all on the Server object.
+
+    Every real AlarmConditionType event on i=2253 maps to PLC_ALARM via the
+    source-level fault_code. A non-condition BaseEventType on the same
+    EventNotifier resolves its ConditionId to null and must be dropped by the
+    poller's guard rather than mapped to PLC_ALARM. Only this one source is
+    mapped, so a real condition (which is also a notifier of i=2253) surfaces
+    exactly one PLC_ALARM fault - no second monitored item, no fault_code race.
+    """
+    path.write_text(
+        'area_id: plc_systems\n'
+        'component_id: alarm_test_runtime\n'
+        'nodes:\n'
+        '  - node_id: "ns=2;s=Tank.Level"\n'
+        '    entity_id: tank_process\n'
+        '    data_name: tank_level\n'
+        '    data_type: float\n'
+        'event_alarms:\n'
+        '  - alarm_source: "ns=0;i=2253"\n'
+        '    entity_id: tank_process\n'
+        '    fault_code: PLC_ALARM\n'
+    )
+
+
 def write_gateway_params(path, *, port, plugin, server_port, node_map, manifest,
                          certs, password):
     """Render a gateway params file pointing the opcua plugin at the secure server."""
@@ -261,7 +290,7 @@ def main():
     server_log = workdir / 'server.log'
     env = dict(os.environ, ROS_DOMAIN_ID=ROS_DOMAIN_ID)
 
-    server = fault_mgr = gw_good = gw_bad = None
+    server = fault_mgr = gw_good = gw_bad = gw_catch = None
     try:
         # --- Certificates (regenerated every run, never committed) ---------
         subprocess.run(['bash', str(gen_certs), str(certs)], check=True,
@@ -376,6 +405,75 @@ def main():
         terminate(gw_good)
         gw_good = None
 
+        # === NON-CONDITION GUARD: explicit event_alarms catch-all on i=2253 =
+        # A catch-all on the Server object maps every real condition to
+        # PLC_ALARM. A non-condition BaseEventType on the SAME EventNotifier
+        # (the fixture's 'sysevent' command) carries a null ConditionId and must
+        # be dropped by the poller's guard - it must never surface as PLC_ALARM,
+        # while a real condition on the catch-all still faults. Reset the still-
+        # active Overpressure condition first so its ConditionRefresh replay does
+        # not pre-map PLC_ALARM before the sysevent is even fired.
+        server.stdin.write('clear Overpressure\n')
+        server.stdin.flush()
+
+        catch_node_map = workdir / 'catchall_nodes.yaml'
+        write_catchall_node_map(catch_node_map)
+        catch_port = free_port()
+        catch_params = workdir / 'gateway_catch.yaml'
+        write_gateway_params(catch_params, port=catch_port, plugin=plugin,
+                             server_port=server_port, node_map=catch_node_map,
+                             manifest=manifest, certs=certs, password=PASSWORD)
+        catch_log = workdir / 'gateway_catch.log'
+        gw_catch = start_gateway(catch_params, catch_log, env)
+        catch_base = f'http://127.0.0.1:{catch_port}/api/v1'
+        catch_status = wait_json(
+            f'{catch_base}/components/alarm_test_runtime/x-plc-status',
+            lambda j: j.get('connected') is True, deadline=70)
+        if not (catch_status and catch_status.get('connected') is True):
+            print('FAIL: catch-all gateway did not connect over the secure channel',
+                  file=sys.stderr)
+            print(catch_log.read_text(errors='replace')[-3000:], file=sys.stderr)
+            return 1
+
+        # Fire the non-condition system event; the guard must drop it.
+        server.stdin.write('sysevent\n')
+        server.stdin.flush()
+        leaked = wait_json(
+            f'{catch_base}/faults',
+            lambda j: any(i.get('fault_code') == NONCOND_CODE for i in j.get('items', [])),
+            deadline=20, period=2.0)
+        if leaked and any(i.get('fault_code') == NONCOND_CODE for i in leaked.get('items', [])):
+            print('FAIL: non-condition event surfaced as PLC_ALARM (guard regressed)',
+                  file=sys.stderr)
+            print(json.dumps(leaked), file=sys.stderr)
+            print('server log:\n' + Path(server_log).read_text(errors='replace'),
+                  file=sys.stderr)
+            return 1
+        print('  OK non-condition guard: sysevent did NOT raise PLC_ALARM')
+
+        # A real condition on the catch-all source still faults (proves the
+        # pipeline can produce PLC_ALARM, so the absence above is meaningful).
+        server.stdin.write('fire Overpressure 750\n')
+        server.stdin.flush()
+        catch_faults = wait_json(
+            f'{catch_base}/faults',
+            lambda j: any(i.get('fault_code') == NONCOND_CODE and i.get('status') == 'CONFIRMED'
+                          for i in j.get('items', [])), deadline=120)
+        got_alarm = catch_faults and any(
+            i.get('fault_code') == NONCOND_CODE and i.get('status') == 'CONFIRMED'
+            for i in catch_faults.get('items', []))
+        if not got_alarm:
+            print('FAIL: real condition on the i=2253 catch-all did not surface as PLC_ALARM',
+                  file=sys.stderr)
+            print(json.dumps(catch_faults), file=sys.stderr)
+            print('server log:\n' + Path(server_log).read_text(errors='replace'),
+                  file=sys.stderr)
+            return 1
+        print('  OK catch-all: real condition surfaced as PLC_ALARM CONFIRMED')
+
+        terminate(gw_catch)
+        gw_catch = None
+
         # === NEGATIVE: wrong password must fail closed =====================
         bad_params = workdir / 'gateway_bad.yaml'
         write_gateway_params(bad_params, port=bad_port, plugin=plugin,
@@ -413,6 +511,7 @@ def main():
         return 0
     finally:
         terminate(gw_good)
+        terminate(gw_catch)
         terminate(gw_bad)
         terminate(fault_mgr)
         if server is not None: