fix(opcua,#386): input validation + cross-pipeline alarm collision check (bburda review)

Two security/correctness gaps from bburda's review on PR #387:

(1) **fault_code / comment unbounded input.** ``execute_operation`` for
``acknowledge_fault`` / ``confirm_fault`` accepted operator input
without length cap or charset filter:

- ``fault_code`` was concatenated into log lines and HTTP error bodies
  via ``"...fault_code=\" + fault_code + \"...\"`` - newlines, control
  chars, or multi-MB blobs would land verbatim in gateway logs.
- ``comment`` was wrapped as ``LocalizedText`` and forwarded verbatim to
  the PLC's condition log - an authenticated operator role could spam
  the PLC with multi-MB blobs on every ack.

Both now validated at the operation entry: ``fault_code`` must match
``is_valid_path_segment`` (alphanumeric + ``_`` + ``-``, 1-256 chars,
matching the existing entity-id bound) and ``comment`` is capped at 256
chars. Failures return HTTP 400 with the actual length so the operator
sees the bound, not a silent truncation.

(2) **Cross-pipeline alarm collision uncaught.** The previous
mutual-exclusion check in ``NodeMap::load`` (which I added in response
to Copilot review) caught ``alarm_source`` misplaced under ``nodes:``,
but missed the genuine collision worth checking: the same
``(entity_id, fault_code)`` declared by both threshold polling
(``nodes[*].alarm``) and event subscription (``event_alarms``).
fault_manager would receive both pipelines' calls and the resulting
status flapping is impossible to debug at runtime; the two paths have
different semantics (debounced vs state-machine) so a merge is not
even well-defined.

``NodeMap::load`` now builds a set of ``(entity_id, fault_code)`` keys
from ``event_alarms`` and rejects the whole file if any
``nodes[*].alarm`` matches a key already claimed by an event alarm.
Two new gtest cases lock the contract:

- ``RejectsThresholdEventAlarmCollision`` - same ``(tank_process,
  PLC_OVERPRESSURE)`` declared by both -> load returns false.
- ``AcceptsDifferentFaultCodesAcrossPipelines`` - same entity but
  distinct fault_codes per pipeline -> load passes (no false-positive).

Local verify: 34/34 test_node_map + 13/13 test_opcua_plugin.

Author: mfaferek93

src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp

@@ -21,9 +21,11 @@
 #include <fstream>
 #include <iostream>
 #include <optional>
+#include <set>
 #include <sstream>
 #include <stdexcept>
 #include <string>
+#include <utility>
 
 #include <nlohmann/json.hpp>
 #include <rclcpp/rclcpp.hpp>
@@ -351,6 +353,30 @@ bool NodeMap::load(const std::string & yaml_path) {
       }
     }
 
+    // Cross-section collision check (bburda review on PR #387). The two
+    // alarm pipelines - threshold polling under ``nodes[*].alarm`` and
+    // native event subscription under ``event_alarms`` - must not address
+    // the same ``(entity_id, fault_code)``: fault_manager would receive
+    // both ``report_fault`` calls and the resulting status flapping is
+    // impossible to debug at runtime. The two paths produce different
+    // semantics (debounced vs state-machine driven) so the merge is not
+    // even well-defined.
+    {
+      std::set<std::pair<std::string, std::string>> event_keys;
+      for (const auto & cfg : event_alarms_) {
+        event_keys.emplace(cfg.entity_id, cfg.fault_code);
+      }
+      for (const auto & entry : entries_) {
+        if (entry.alarm.has_value() && event_keys.count({entry.entity_id, entry.alarm->fault_code}) > 0) {
+          RCLCPP_ERROR(rclcpp::get_logger("opcua.node_map"),
+                       "Duplicate (entity_id=%s, fault_code=%s) declared by both nodes[*].alarm "
+                       "(threshold mode) and event_alarms[*] (subscription mode) - mutually exclusive",
+                       entry.entity_id.c_str(), entry.alarm->fault_code.c_str());
+          return false;
+        }
+      }
+    }
+
     build_entity_defs();
     return true;
 

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp

@@ -909,6 +909,27 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string
     std::string fault_code = parameters["fault_code"].get<std::string>();
     std::string comment = parameters.value("comment", std::string{});
 
+    // Input validation (bburda review on PR #387). ``fault_code`` lands in
+    // log lines and HTTP error bodies via string concatenation; without
+    // length / charset bounds an authenticated operator can inject
+    // newlines, control chars, or multi-MB blobs into gateway logs.
+    // ``comment`` is forwarded verbatim to the PLC as ``LocalizedText`` -
+    // unbounded length lets the same role spam the PLC's condition log.
+    // 256 chars matches the existing entity-id bound (``is_valid_path_segment``)
+    // and is generous enough for typical ``PLC_OVERPRESSURE_SENSOR_3`` style
+    // identifiers + a one-sentence operator comment.
+    if (!is_valid_path_segment(fault_code)) {
+      return tl::make_unexpected(OperationProviderErrorInfo{OperationProviderError::InvalidParameters,
+                                                            "fault_code must be alphanumeric+_- and 1-256 chars (got " +
+                                                                std::to_string(fault_code.size()) + " chars)",
+                                                            400});
+    }
+    if (comment.size() > 256) {
+      return tl::make_unexpected(
+          OperationProviderErrorInfo{OperationProviderError::InvalidParameters,
+                                     "comment exceeds 256 chars (got " + std::to_string(comment.size()) + ")", 400});
+    }
+
     auto runtime = poller_->lookup_condition(entity_id, fault_code);
     if (!runtime) {
       return tl::make_unexpected(OperationProviderErrorInfo{

src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp

@@ -521,6 +521,66 @@ component_id: test
   EXPECT_TRUE(map.entries()[0].alarm->above_threshold);
 }
 
+TEST_F(NodeMapTest, RejectsThresholdEventAlarmCollision) {
+  // bburda review on PR #387: the genuine schema collision worth checking
+  // is not "alarm_source under nodes" (covered separately) but the same
+  // ``(entity_id, fault_code)`` declared by both a threshold alarm under
+  // ``nodes[*].alarm`` and a subscription entry under ``event_alarms``.
+  // fault_manager would receive both pipelines' calls and the resulting
+  // status flapping is impossible to debug at runtime; the two paths have
+  // different semantics (debounced vs state-machine) so a merge is not
+  // even well-defined. Loader rejects the whole file.
+  std::string path = "/tmp/test_node_map_alarm_collision.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+nodes:
+  - node_id: "ns=1;i=1"
+    entity_id: tank_process
+    data_name: pressure
+    alarm:
+      fault_code: PLC_OVERPRESSURE
+      threshold: 90.0
+event_alarms:
+  - alarm_source: "ns=2;s=Alarms.Overpressure"
+    entity_id: tank_process
+    fault_code: PLC_OVERPRESSURE
+)";
+  f.close();
+
+  NodeMap map;
+  EXPECT_FALSE(map.load(path));
+}
+
+TEST_F(NodeMapTest, AcceptsDifferentFaultCodesAcrossPipelines) {
+  // Same entity, *different* fault_codes across pipelines is fine - each
+  // fault_manager entry is keyed on fault_code, so no collision. Locks
+  // the contract that the rejection above is precise (won't false-positive
+  // when only entity overlaps).
+  std::string path = "/tmp/test_node_map_alarm_no_collision.yaml";
+  std::ofstream f(path);
+  f << R"(
+area_id: test
+component_id: test
+nodes:
+  - node_id: "ns=1;i=1"
+    entity_id: tank_process
+    data_name: pressure
+    alarm:
+      fault_code: PLC_PRESSURE_HIGH
+      threshold: 90.0
+event_alarms:
+  - alarm_source: "ns=2;s=Alarms.Overheat"
+    entity_id: tank_process
+    fault_code: PLC_OVERHEAT
+)";
+  f.close();
+
+  NodeMap map;
+  EXPECT_TRUE(map.load(path));
+}
+
 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