fix(opcua): accept severity as an alias in event_alarms

src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp

@@ -29,6 +29,7 @@
 #include <ros2_medkit_gateway/plugins/ros_plugin_context.hpp>
 
 #include <atomic>
+#include <cstdint>
 #include <memory>
 #include <string>
 #include <unordered_map>
@@ -110,6 +111,12 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin,
   tl::expected<dto::FaultClearResult, FaultProviderErrorInfo> clear_fault(const std::string & entity_id,
                                                                           const std::string & fault_code) override;
 
+  // Resolve the SOVD severity bucket for an event alarm. An explicit configured
+  // override wins; with none configured the raw OPC-UA event Severity (1-1000)
+  // is mapped to a bucket by band (>=801 CRITICAL, >=501 ERROR, >=201 WARNING,
+  // else INFO). Pure + static so it is unit-testable without a live server.
+  static std::string map_severity(uint16_t live_severity, const std::string & severity_override);
+
  private:
   // Route handlers
   void handle_plc_data(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res);

src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp

@@ -19,6 +19,7 @@
 #include <cctype>
 #include <cstdint>
 #include <fstream>
+#include <initializer_list>
 #include <iostream>
 #include <optional>
 #include <sstream>
@@ -569,6 +570,56 @@ bool NodeMap::load(const std::string & yaml_path) {
                      "event_alarms has %zu entries (max 10000) - refusing to load", event_alarms_node.size());
         return false;
       }
+      // Resolve the configured severity for an event_alarms entry / mapping.
+      // ``severity`` is accepted as an alias for ``severity_override`` because
+      // the threshold ``alarm:`` block uses ``severity:``; an operator who
+      // reuses that key here must not have it silently dropped. When both are
+      // present ``severity_override`` wins. An explicit value is validated like
+      // the threshold path; with neither key the result stays empty so the live
+      // event Severity band mapping applies (see OpcuaPlugin::map_severity).
+      auto read_severity_override = [&](const YAML::Node & node, const std::string & context) -> std::string {
+        // yaml-cpp reports a present-but-null key (``severity_override:`` with
+        // no value / ``~``) as defined/truthy; gate on a real value so a null
+        // override neither shadows a valid ``severity:`` alias nor emits a
+        // misleading "non-string severity_override" warning.
+        auto has_value = [](const YAML::Node & n) {
+          return n.IsDefined() && !n.IsNull();
+        };
+        const YAML::Node override_node = node["severity_override"];
+        const bool use_override = has_value(override_node);
+        const YAML::Node sev_node = use_override ? override_node : node["severity"];
+        if (!has_value(sev_node)) {
+          return "";
+        }
+        const char * field = use_override ? "severity_override" : "severity";
+        auto sev = parse_string(sev_node, field, context);
+        if (!sev || sev->empty()) {
+          return "";
+        }
+        return validate_severity(*sev, context);
+      };
+      // Warn on keys the event_alarms loader does not recognise so a future
+      // typo (e.g. ``severty:``) is surfaced instead of silently dropped.
+      auto warn_unknown_keys = [logger](const YAML::Node & node, const std::string & context,
+                                        std::initializer_list<const char *> known) {
+        if (!node.IsMap()) {
+          return;
+        }
+        for (const auto & kv : node) {
+          std::string key;
+          try {
+            key = kv.first.as<std::string>();
+          } catch (const YAML::Exception &) {
+            continue;
+          }
+          const bool recognised = std::any_of(known.begin(), known.end(), [&key](const char * k) {
+            return key == k;
+          });
+          if (!recognised) {
+            RCLCPP_WARN(logger, "%s: unknown key '%s' - ignored", context.c_str(), key.c_str());
+          }
+        }
+      };
       for (size_t i = 0; i < event_alarms_node.size(); ++i) {
         const auto & a = event_alarms_node[i];
         if (!a["alarm_source"] || !a["entity_id"]) {
@@ -596,12 +647,15 @@ bool NodeMap::load(const std::string & yaml_path) {
           }
           alarm_fault_code = *parsed_fault_code;
         }
+        warn_unknown_keys(a, alarm_label,
+                          {"alarm_source", "entity_id", "fault_code", "severity_override", "severity", "message",
+                           "mappings", "associated_values"});
         AlarmEventConfig cfg;
         cfg.source_node_id_str = *alarm_source;
         cfg.source_node_id = parse_node_id(cfg.source_node_id_str);
         cfg.entity_id = *alarm_entity_id;
         cfg.fault_code = alarm_fault_code;
-        cfg.severity_override = a["severity_override"].as<std::string>("");
+        cfg.severity_override = read_severity_override(a, alarm_label);
         cfg.message_override = a["message"].as<std::string>("");
 
         // Issue #389: per-condition-identity mappings (multi-alarm).
@@ -616,12 +670,16 @@ bool NodeMap::load(const std::string & yaml_path) {
             if (!mapping_fault_code) {
               continue;
             }
+            const std::string mapping_label = alarm_label + " mapping";
+            warn_unknown_keys(m, mapping_label,
+                              {"condition_name", "source_node", "event_type", "fault_code", "severity_override",
+                               "severity", "message"});
             AlarmMapping mapping;
             mapping.match_condition_name = m["condition_name"].as<std::string>("");
             mapping.match_source_node = m["source_node"].as<std::string>("");
             mapping.match_event_type = m["event_type"].as<std::string>("");
             mapping.fault_code = *mapping_fault_code;
-            mapping.severity_override = m["severity_override"].as<std::string>("");
+            mapping.severity_override = read_severity_override(m, mapping_label);
             mapping.message_override = m["message"].as<std::string>("");
             cfg.mappings.push_back(std::move(mapping));
           }

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp

@@ -692,30 +692,32 @@ void OpcuaPlugin::on_alarm_change(const std::string & entity_id,
   }
 }
 
+std::string OpcuaPlugin::map_severity(uint16_t live_severity, const std::string & severity_override) {
+  // Map raw OPC-UA Severity (1-1000) to SOVD severity bucket.
+  // Selfpatch convention documented in design/index.rst; not from IEC 62682.
+  // The resolved severity_override (mapping- or source-level, issue #389)
+  // wins when set.
+  if (!severity_override.empty()) {
+    return severity_override;
+  }
+  if (live_severity >= 801) {
+    return std::string("CRITICAL");
+  }
+  if (live_severity >= 501) {
+    return std::string("ERROR");
+  }
+  if (live_severity >= 201) {
+    return std::string("WARNING");
+  }
+  return std::string("INFO");
+}
+
 void OpcuaPlugin::on_event_alarm(const AlarmEventDelivery & delivery) {
   if (shutdown_requested_.load()) {
     return;
   }
 
-  // Map raw OPC-UA Severity (1-1000) to SOVD severity bucket.
-  // Selfpatch convention documented in design/index.rst; not from IEC 62682.
-  // The resolved severity_override (mapping- or source-level, issue #389)
-  // wins when set.
-  auto severity_str = [&]() {
-    if (!delivery.severity_override.empty()) {
-      return delivery.severity_override;
-    }
-    if (delivery.severity >= 801) {
-      return std::string("CRITICAL");
-    }
-    if (delivery.severity >= 501) {
-      return std::string("ERROR");
-    }
-    if (delivery.severity >= 201) {
-      return std::string("WARNING");
-    }
-    return std::string("INFO");
-  }();
+  const std::string severity_str = map_severity(delivery.severity, delivery.severity_override);
 
   switch (delivery.action) {
     case AlarmAction::ReportConfirmed:

src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp

@@ -584,6 +584,124 @@ component_id: test
   EXPECT_TRUE(map.load(path));
 }
 
+TEST_F(NodeMapTest, EventAlarmSeverityAliasSetsOverride) {
+  // Regression: an ``event_alarms`` entry configured with ``severity:`` (the
+  // key the threshold ``alarm:`` block uses) must not be silently dropped. It
+  // is accepted as an alias for ``severity_override`` so the configured
+  // severity reaches the fault instead of falling back to the live event
+  // Severity band mapping.
+  std::string path = "/tmp/test_node_map_event_alarm_severity_alias.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+event_alarms:
+  - alarm_source: "ns=3;s=Program_Alarm"
+    entity_id: plc_program
+    fault_code: PLC_PROGRAM_ALARM
+    severity: ERROR
+)";
+  f.close();
+
+  NodeMap map;
+  ASSERT_TRUE(map.load(path));
+  ASSERT_EQ(map.event_alarms().size(), 1u);
+  EXPECT_EQ(map.event_alarms()[0].severity_override, "ERROR");
+}
+
+TEST_F(NodeMapTest, EventAlarmSeverityOverrideWinsOverAlias) {
+  // ``severity_override`` is the canonical key; when both are present it wins.
+  std::string path = "/tmp/test_node_map_event_alarm_severity_precedence.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+event_alarms:
+  - alarm_source: "ns=3;s=Program_Alarm"
+    entity_id: plc_program
+    fault_code: PLC_PROGRAM_ALARM
+    severity_override: CRITICAL
+    severity: WARNING
+)";
+  f.close();
+
+  NodeMap map;
+  ASSERT_TRUE(map.load(path));
+  ASSERT_EQ(map.event_alarms().size(), 1u);
+  EXPECT_EQ(map.event_alarms()[0].severity_override, "CRITICAL");
+}
+
+TEST_F(NodeMapTest, EventAlarmMappingSeverityAliasSetsOverride) {
+  // The alias also applies to per-condition ``mappings`` entries (issue #389).
+  std::string path = "/tmp/test_node_map_event_alarm_mapping_severity_alias.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+event_alarms:
+  - alarm_source: "ns=3;s=Program_Alarm"
+    entity_id: plc_program
+    mappings:
+      - condition_name: Overtemp
+        fault_code: PLC_OVERTEMP
+        severity: WARNING
+)";
+  f.close();
+
+  NodeMap map;
+  ASSERT_TRUE(map.load(path));
+  ASSERT_EQ(map.event_alarms().size(), 1u);
+  ASSERT_EQ(map.event_alarms()[0].mappings.size(), 1u);
+  EXPECT_EQ(map.event_alarms()[0].mappings[0].severity_override, "WARNING");
+}
+
+TEST_F(NodeMapTest, EventAlarmNullSeverityOverrideFallsBackToAlias) {
+  // A present-but-null ``severity_override:`` is reported as defined by yaml-cpp
+  // but must not shadow a real ``severity:`` sibling (nor emit a spurious
+  // "non-string severity_override" warning): the alias still wins.
+  std::string path = "/tmp/test_node_map_event_alarm_null_override.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+event_alarms:
+  - alarm_source: "ns=3;s=Program_Alarm"
+    entity_id: plc_program
+    fault_code: PLC_PROGRAM_ALARM
+    severity_override:
+    severity: WARNING
+)";
+  f.close();
+
+  NodeMap map;
+  ASSERT_TRUE(map.load(path));
+  ASSERT_EQ(map.event_alarms().size(), 1u);
+  EXPECT_EQ(map.event_alarms()[0].severity_override, "WARNING");
+}
+
+TEST_F(NodeMapTest, EventAlarmSeverityAliasInvalidBucketDefaultsToError) {
+  // The alias is *validated* (validate_severity), not passed through raw: an
+  // unknown bucket is defaulted to ERROR so a typo cannot misroute the fault to
+  // a bogus severity. Pins the validation behaviour the alias path adds.
+  std::string path = "/tmp/test_node_map_event_alarm_severity_invalid.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+event_alarms:
+  - alarm_source: "ns=3;s=Program_Alarm"
+    entity_id: plc_program
+    fault_code: PLC_PROGRAM_ALARM
+    severity: notabucket
+)";
+  f.close();
+
+  NodeMap map;
+  ASSERT_TRUE(map.load(path));
+  ASSERT_EQ(map.event_alarms().size(), 1u);
+  EXPECT_EQ(map.event_alarms()[0].severity_override, "ERROR");
+}
+
 TEST_F(NodeMapTest, RejectsAlarmSourceUnderNodes) {
   // Schema validation: ``alarm_source`` is only valid in the top-level
   // ``event_alarms:`` section. Used to be silently ignored when not paired

src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp

@@ -502,4 +502,30 @@ TEST(ReadSnapshotClassify, ActiveButFlaggedUnreliableIsKeptNotFed) {
   EXPECT_EQ(d, OpcuaPoller::ReadReplayDisposition::KeepOnly);
 }
 
+// -- OpcuaPlugin::map_severity (event-alarm severity resolution) -------------
+
+TEST(MapSeverity, ConfiguredOverrideWinsOverLiveSeverity) {
+  // An explicit configured severity wins regardless of the live event Severity.
+  EXPECT_EQ(OpcuaPlugin::map_severity(1000, "INFO"), "INFO");
+  EXPECT_EQ(OpcuaPlugin::map_severity(1, "CRITICAL"), "CRITICAL");
+}
+
+TEST(MapSeverity, LowLiveSeverityMapsToInfo) {
+  // The Siemens trap: many servers emit Program_Alarm events at Severity 1, so
+  // with no configured override the fault lands as INFO. This is exactly why a
+  // silently dropped ``severity:`` key mattered.
+  EXPECT_EQ(OpcuaPlugin::map_severity(1, ""), "INFO");
+}
+
+TEST(MapSeverity, LiveSeverityBandBoundaries) {
+  // Band map: >=801 CRITICAL, >=501 ERROR, >=201 WARNING, else INFO.
+  EXPECT_EQ(OpcuaPlugin::map_severity(200, ""), "INFO");
+  EXPECT_EQ(OpcuaPlugin::map_severity(201, ""), "WARNING");
+  EXPECT_EQ(OpcuaPlugin::map_severity(500, ""), "WARNING");
+  EXPECT_EQ(OpcuaPlugin::map_severity(501, ""), "ERROR");
+  EXPECT_EQ(OpcuaPlugin::map_severity(800, ""), "ERROR");
+  EXPECT_EQ(OpcuaPlugin::map_severity(801, ""), "CRITICAL");
+  EXPECT_EQ(OpcuaPlugin::map_severity(1000, ""), "CRITICAL");
+}
+
 }  // namespace ros2_medkit_gateway