fix(opcua,#386): Copilot review feedback batch (observability + hygiene)

Eight Copilot findings on PR #387 addressed in one batch:

Observability (#6/#7/#8/#10): every per-event / per-method-call
``std::cerr`` trace is now gated by the ``ROS2_MEDKIT_OPCUA_TRACE``
env-var (off by default). Covered the trampoline, ``add_event_monitored_item``,
``call_method``, ``on_event``, EventId capture, state-machine
classification, dispatch trace, and the SOVD ack/confirm EventId hex
dump. Production logs now stay clean; integration-test debugging
re-enables verbose mode with one env-var. Operator-visible
``[opcua_poller WARN] ConditionRefresh rejected`` stays unconditional.

Comment / code mismatches (#1/#11/#12):
- ``OpcuaPlugin::on_event_alarm`` ReportHealed branch now explicitly
  documents the intentional no-op and explains why pushing EVENT_PASSED
  to fault_manager would defeat the OPC-UA Part 9 ack/confirm contract.
- ``alarm_state_machine.hpp`` Retain comment matches reality - we do
  not currently strip Retain=false events because the EventFilter
  doesn't include Retain in its select clauses (followup issue #389).

Test fixes:
- (#3) ``DisabledNoOpWhenAlreadyCleared`` renamed to
  ``DisabledTransitionsClearedToSuppressedNoOp`` so the name reflects
  both the status transition (Cleared -> Suppressed) and the no-op
  action.
- (#13) New ``NodeMapTest.RejectsAlarmSourceUnderNodes``: locks the
  schema validation that rejects misplaced ``alarm_source`` keys under
  ``nodes:`` (was silently ignored), pointing the user at
  ``event_alarms:``.

Schema validation (#13): ``NodeMap::load`` now rejects any
``alarm_source`` key under ``nodes:`` with an actionable error message.
Previous behaviour silently ignored the typo unless paired with
``alarm.threshold``, leaving "subscribed alarm that never fires" cases
impossible to diagnose at runtime.

Test/script hygiene:
- (#9) ``run_alarm_tests.sh`` replaces the fixed ``sleep 3`` between
  ``fault_manager_node`` start and ``gateway_node`` start with a 30-try
  poll on ``ros2 service list | grep /fault_manager/report_fault``.
  Matches the project rule "no fixed sleeps".
- (#2) ``run_ctest.py`` smoke-test runner: skips on missing ``asyncua``
  by default (preserves local dev iteration), but treats the env-var
  ``ROS2_MEDKIT_OPCUA_SMOKE_REQUIRE=1`` as "fail hard" so a CI step
  that drops the ``asyncua`` install cannot silently bypass the smoke
  check.

Local verify: 27/27 test_opcua_client + 27/27 test_alarm_state_machine
+ 1/1 new RejectsAlarmSourceUnderNodes.

Author: mfaferek93

src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh

@@ -267,7 +267,17 @@ docker run -d --name "${GATEWAY_NAME}" --network "${NET_NAME}" \
     # the gateway opcua plugin tries to call /fault_manager/report_fault.
     ros2 run ros2_medkit_fault_manager fault_manager_node \
       > /var/lib/ros2_medkit/fault_manager.log 2>&1 &
-    sleep 3
+    # Poll for service advertisement instead of a fixed sleep (Copilot
+    # review on PR #387). On slow CI runners the previous ``sleep 3`` was
+    # sometimes too short, leaving the gateway to come up before the
+    # service was discoverable. ``ros2 service list`` is the cheapest
+    # ROS-native availability signal.
+    for i in $(seq 1 30); do
+      if ros2 service list 2>/dev/null | grep -q "/fault_manager/report_fault"; then
+        break
+      fi
+      sleep 0.2
+    done
     PLUGIN_PATH=$(find /root/ws/install -name "libros2_medkit_opcua_plugin.so" | head -1)
     exec ros2 run ros2_medkit_gateway gateway_node \
       --ros-args --params-file /config/gateway_params.yaml \

src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/alarm_state_machine.hpp

@@ -68,10 +68,13 @@ struct AlarmEventInput {
 ///   5. ActiveState == false          -> Healed or Cleared based on
 ///                                       Acked + Confirmed
 ///
-/// Retain is intentionally NOT used here. Per Part 9 §5.5.2.10 it controls
-/// visibility during ConditionRefresh bursts, not lifecycle - the poller
-/// strips Retain=false events delivered between RefreshStartEvent and
-/// RefreshEndEvent before invoking compute().
+/// Retain is intentionally not modeled by this state machine and does not
+/// affect ``compute()``. Per Part 9 §5.5.2.10 it controls visibility during
+/// ConditionRefresh bursts rather than the lifecycle mapping implemented
+/// here. The current EventFilter does not include Retain in its select
+/// clauses; if/when ConditionRefresh-with-Retain filtering is added (issue
+/// #389), it will live in the poller's pre-compute path, not in this
+/// pure-function table. (Copilot review on PR #387.)
 class AlarmStateMachine {
  public:
   struct Outcome {

src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp

@@ -334,14 +334,18 @@ bool NodeMap::load(const std::string & yaml_path) {
       }
     }
 
-    // Mutual-exclusion check: an entry under ``nodes:`` carrying both a
-    // ``threshold`` alarm and an ``alarm_source`` is a configuration error
-    // (the threshold path polls scalar values; the alarm_source path
-    // subscribes to native events). Reject the whole file rather than guess.
+    // Schema validation under ``nodes:``: ``alarm_source`` belongs in the
+    // top-level ``event_alarms:`` section, never under ``nodes:``. Silently
+    // ignoring a misplaced ``alarm_source`` (the previous behavior unless
+    // also paired with ``alarm.threshold``) lets a configuration typo land
+    // a "subscribed alarm that never fires", which is impossible to
+    // diagnose from runtime logs. Reject the whole file with an actionable
+    // error pointing at the right place. (Copilot review on PR #387.)
     for (const auto & node : (nodes ? nodes : YAML::Node{})) {
-      if (node["alarm_source"] && node["alarm"] && node["alarm"]["threshold"]) {
+      if (node["alarm_source"]) {
         RCLCPP_ERROR(rclcpp::get_logger("opcua.node_map"),
-                     "Entry node_id=%s declares both threshold alarm and alarm_source - mutually exclusive",
+                     "Entry node_id=%s uses ``alarm_source`` under ``nodes:``, which is invalid; "
+                     "move this configuration to top-level ``event_alarms:`` (see README §event_alarms)",
                      node["node_id"] ? node["node_id"].as<std::string>().c_str() : "<unknown>");
         return false;
       }

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp

@@ -16,6 +16,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <cstdlib>
 #include <cstring>
 #include <deque>
 #include <iostream>
@@ -29,6 +30,20 @@
 
 namespace ros2_medkit_gateway {
 
+// Env-var gate for verbose per-event / per-method-call diagnostics.
+// Set ``ROS2_MEDKIT_OPCUA_TRACE=1`` to enable trampoline / EventId /
+// call_method stderr logging. Off by default to keep production logs sane
+// (Copilot review on PR #387: per-event std::cerr would flood under high
+// alarm rates and bypass the gateway's normal logging path). Resolved once
+// at process start so toggling requires a restart.
+inline bool opcua_trace_enabled() {
+  static const bool enabled = []() {
+    const char * v = std::getenv("ROS2_MEDKIT_OPCUA_TRACE");
+    return v != nullptr && v[0] != '\0' && std::string(v) != "0";
+  }();
+  return enabled;
+}
+
 // Forward declaration - defined after Impl so the trampoline can call back into
 // the client. Static linkage keeps the symbol private to this translation unit.
 struct EventCallbackContext;
@@ -599,11 +614,15 @@ UA_EventFilter make_event_filter(const std::vector<OpcuaClient::EventFieldSpec>
 // Defined at namespace scope so its address is a stable function pointer.
 static void on_event_trampoline_c(UA_Client * /*client*/, UA_UInt32 sub_id, void * /*sub_ctx*/, UA_UInt32 mon_id,
                                   void * mon_ctx, size_t n_fields, UA_Variant * fields) {
-  std::cerr << "[opcua_client] TRAMPOLINE FIRED sub=" << sub_id << " mon=" << mon_id << " n_fields=" << n_fields
-            << std::endl;
+  if (opcua_trace_enabled()) {
+    std::cerr << "[opcua_client] TRAMPOLINE FIRED sub=" << sub_id << " mon=" << mon_id << " n_fields=" << n_fields
+              << std::endl;
+  }
   auto * ctx = static_cast<EventCallbackContext *>(mon_ctx);
   if (ctx == nullptr || ctx->owner == nullptr) {
-    std::cerr << "[opcua_client] TRAMPOLINE: ctx null" << std::endl;
+    if (opcua_trace_enabled()) {
+      std::cerr << "[opcua_client] TRAMPOLINE: ctx null" << std::endl;
+    }
     return;
   }
   // Stale callback from a defunct subscription - ctx is still valid (we only
@@ -710,11 +729,13 @@ uint32_t OpcuaClient::add_event_monitored_item(uint32_t subscription_id, const o
   item.requestedParameters.queueSize = 100;
   UA_ExtensionObject_setValueNoDelete(&item.requestedParameters.filter, &filter, &UA_TYPES[UA_TYPES_EVENTFILTER]);
 
-  // Debug log so integration-test failures surface the exact NodeId we
-  // hand to the server. Trace-level diagnostic; can be tightened to a
-  // ROS RCLCPP_DEBUG once the issue #386 server interop is stable.
-  std::cerr << "[opcua_client] add_event_monitored_item: subId=" << subscription_id
-            << " nodeId=" << source_node.toString() << " selectClauses=" << (select_specs.size() + 3) << std::endl;
+  // Trace-level diagnostic gated by ROS2_MEDKIT_OPCUA_TRACE so integration
+  // test failures can surface the exact NodeId / select-clause count we
+  // hand to the server, without flooding production stderr.
+  if (opcua_trace_enabled()) {
+    std::cerr << "[opcua_client] add_event_monitored_item: subId=" << subscription_id
+              << " nodeId=" << source_node.toString() << " selectClauses=" << (select_specs.size() + 3) << std::endl;
+  }
 
   UA_MonitoredItemCreateResult result =
       UA_Client_MonitoredItems_createEvent(impl_->client.handle(), subscription_id, UA_TIMESTAMPSTORETURN_BOTH, item,
@@ -731,8 +752,10 @@ uint32_t OpcuaClient::add_event_monitored_item(uint32_t subscription_id, const o
   UA_MonitoredItemCreateRequest_clear(&item);
   UA_EventFilter_clear(&filter);
 
-  std::cerr << "[opcua_client] createEvent result: status=" << UA_StatusCode_name(result.statusCode)
-            << " miId=" << result.monitoredItemId << std::endl;
+  if (opcua_trace_enabled()) {
+    std::cerr << "[opcua_client] createEvent result: status=" << UA_StatusCode_name(result.statusCode)
+              << " miId=" << result.monitoredItemId << std::endl;
+  }
 
   if (result.statusCode != UA_STATUSCODE_GOOD) {
     UA_MonitoredItemCreateResult_clear(&result);
@@ -836,16 +859,19 @@ OpcuaClient::call_method(const opcua::NodeId & object_id, const opcua::NodeId &
         opcua::services::call(impl_->client, object_id, method_id, opcua::Span<const opcua::Variant>(input_args));
     UA_StatusCode code = result.getStatusCode().get();
     auto arg_results = result.getInputArgumentResults();
-    std::cerr << "[opcua_client] call_method object=" << object_id.toString() << " method=" << method_id.toString()
-              << " statusCode=" << UA_StatusCode_name(code);
     std::vector<uint32_t> arg_codes;
     arg_codes.reserve(arg_results.size());
     for (size_t i = 0; i < arg_results.size(); ++i) {
-      uint32_t arg_code = arg_results[i].get();
-      arg_codes.push_back(arg_code);
-      std::cerr << " arg" << i << "=" << UA_StatusCode_name(arg_code);
+      arg_codes.push_back(arg_results[i].get());
+    }
+    if (opcua_trace_enabled()) {
+      std::cerr << "[opcua_client] call_method object=" << object_id.toString() << " method=" << method_id.toString()
+                << " statusCode=" << UA_StatusCode_name(code);
+      for (size_t i = 0; i < arg_codes.size(); ++i) {
+        std::cerr << " arg" << i << "=" << UA_StatusCode_name(arg_codes[i]);
+      }
+      std::cerr << std::endl;
     }
-    std::cerr << std::endl;
 
     auto classified = classify_call_result(code, arg_codes);
     if (!classified.has_value()) {

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp

@@ -33,6 +33,18 @@ namespace ros2_medkit_gateway {
 
 namespace {
 
+// Env-var gate for verbose per-event / per-method-call diagnostics.
+// See opcua_client.cpp for rationale (Copilot review on PR #387). Duplicated
+// to keep traces local to their dispatch sites; promoting to a public header
+// would expose an internal trace knob.
+inline bool opcua_trace_enabled() {
+  static const bool enabled = []() {
+    const char * v = std::getenv("ROS2_MEDKIT_OPCUA_TRACE");
+    return v != nullptr && v[0] != '\0' && std::string(v) != "0";
+  }();
+  return enabled;
+}
+
 /// Parse a JSON "value" field, coerce to the node's declared data_type, and
 /// validate against the optional min/max range. Shared by handle_plc_operations,
 /// DataProvider::write_data, and OperationProvider::execute_operation to keep
@@ -534,14 +546,27 @@ void OpcuaPlugin::on_event_alarm(const AlarmEventDelivery & delivery) {
       send_report_fault(delivery.entity_id, delivery.fault_code, severity_str, delivery.message);
       break;
     case AlarmAction::ReportHealed:
-      // Fault is latched: condition is no longer active but not yet
-      // confirmed. We don't have a dedicated HEALED reporting verb in
-      // ReportFault.srv (only FAILED/PASSED), so we mark this as a PASSED
-      // event - fault_manager keeps the entry in HEALED state until
-      // confirmed, mirroring the lifecycle.
+      // Intentional no-op (Copilot review on PR #387).
+      //
+      // OPC-UA AlarmConditionType HEALED means "alarm physically cleared
+      // (ActiveState=false) but operator workflow incomplete (ack and/or
+      // confirm pending)". Per Part 9 §5.7 the Cleared transition is
+      // operator-driven, not statistical.
+      //
+      // ros2_medkit_msgs/srv/ReportFault has only FAILED/PASSED verbs and
+      // fault_manager treats PASSED through a debounce engine. Sending
+      // EVENT_PASSED on every latch would let fault_manager auto-clear
+      // the fault via healing_threshold debounce, defeating the spec
+      // contract that requires explicit operator Acknowledge + Confirm.
+      // Conversely, healing_enabled=false would silently lose the HEALED
+      // signal entirely.
+      //
+      // Until we add STATUS_LATCHED (or a similar lifecycle-distinguishing
+      // status) to ros2_medkit_msgs/msg/Fault we keep status=CONFIRMED
+      // until the next ClearFault fires. The operator-side gap (cannot
+      // see "physically cleared, awaiting confirm" in the UI) is tracked
+      // separately; see PR #387 review thread.
       log_info("AlarmCondition HEALED (latched, awaiting ack/confirm): " + delivery.fault_code);
-      // No-op for now; fault_manager will keep the fault HEALED until
-      // CLEARED. The state transition is observable via /faults/stream.
       break;
     case AlarmAction::ClearFault:
       log_info("AlarmCondition CLEARED: " + delivery.fault_code);
@@ -898,7 +923,7 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string
     args.push_back(opcua::Variant::fromScalar(runtime->latest_event_id));
     args.push_back(opcua::Variant::fromScalar(opcua::LocalizedText("", comment)));
 
-    {
+    if (opcua_trace_enabled()) {
       const auto * bytes = runtime->latest_event_id.data();
       std::cerr << "[opcua_plugin] " << operation_name << " EventId len=" << runtime->latest_event_id.length()
                 << " hex=";

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_poller.cpp

@@ -17,6 +17,7 @@
 #include <algorithm>
 #include <cmath>
 #include <cstdio>
+#include <cstdlib>
 #include <iostream>
 #include <optional>
 #include <stdexcept>
@@ -26,6 +27,19 @@
 
 namespace ros2_medkit_gateway {
 
+namespace {
+// See opcua_client.cpp for the canonical helper. Duplicated here so the
+// poller's per-event traces stay together with the dispatch they describe;
+// promoting to a public header would expose an internal trace knob.
+inline bool opcua_trace_enabled() {
+  static const bool enabled = []() {
+    const char * v = std::getenv("ROS2_MEDKIT_OPCUA_TRACE");
+    return v != nullptr && v[0] != '\0' && std::string(v) != "0";
+  }();
+  return enabled;
+}
+}  // namespace
+
 namespace {
 
 // EventType NodeIds for ConditionRefresh bracketing per OPC-UA Part 9 §5.5.7
@@ -300,8 +314,10 @@ void OpcuaPoller::condition_refresh() {
 void OpcuaPoller::on_event(const AlarmEventConfig & cfg, const std::vector<opcua::Variant> & values,
                            const opcua::NodeId & /*source_node*/, const opcua::NodeId & event_type,
                            const opcua::NodeId & condition_id) {
-  std::cerr << "[opcua_poller] on_event fault=" << cfg.fault_code << " event_type=" << event_type.toString()
-            << " condition=" << condition_id.toString() << " values=" << values.size() << std::endl;
+  if (opcua_trace_enabled()) {
+    std::cerr << "[opcua_poller] on_event fault=" << cfg.fault_code << " event_type=" << event_type.toString()
+              << " condition=" << condition_id.toString() << " values=" << values.size() << std::endl;
+  }
   // Detect ConditionRefresh bracketing per Part 9 §5.5.7. The flag is for
   // diagnostics only; the state machine itself does not need to know
   // because RefreshStart / RefreshEnd notifications carry no condition
@@ -386,24 +402,28 @@ void OpcuaPoller::on_event(const AlarmEventConfig & cfg, const std::vector<opcua
     // Track the latest EventId for spec-compliant Acknowledge calls.
     if (values[kFieldEventId].isType<opcua::ByteString>()) {
       it->second.latest_event_id = values[kFieldEventId].getScalarCopy<opcua::ByteString>();
-      std::cerr << "[opcua_poller] captured EventId len=" << it->second.latest_event_id.length() << " hex=";
-      const auto * bytes = it->second.latest_event_id.data();
-      for (size_t i = 0; i < std::min<size_t>(it->second.latest_event_id.length(), 16); ++i) {
-        char buf[3];
-        std::snprintf(buf, sizeof(buf), "%02x", static_cast<unsigned>(bytes[i]) & 0xffu);
-        std::cerr << buf;
+      if (opcua_trace_enabled()) {
+        std::cerr << "[opcua_poller] captured EventId len=" << it->second.latest_event_id.length() << " hex=";
+        const auto * bytes = it->second.latest_event_id.data();
+        for (size_t i = 0; i < std::min<size_t>(it->second.latest_event_id.length(), 16); ++i) {
+          char buf[3];
+          std::snprintf(buf, sizeof(buf), "%02x", static_cast<unsigned>(bytes[i]) & 0xffu);
+          std::cerr << buf;
+        }
+        std::cerr << std::endl;
       }
-      std::cerr << std::endl;
-    } else {
+    } else if (opcua_trace_enabled()) {
       std::cerr << "[opcua_poller] EventId field not a ByteString" << std::endl;
     }
 
     auto outcome = AlarmStateMachine::compute(prev_status, input);
-    std::cerr << "[opcua_poller] state machine: enabled=" << input.enabled_state << " active=" << input.active_state
-              << " acked=" << input.acked_state << " confirmed=" << input.confirmed_state
-              << " shelved=" << input.shelved << " branch=" << input.branch_id_present
-              << " prev=" << static_cast<int>(prev_status) << " action=" << static_cast<int>(outcome.action)
-              << std::endl;
+    if (opcua_trace_enabled()) {
+      std::cerr << "[opcua_poller] state machine: enabled=" << input.enabled_state << " active=" << input.active_state
+                << " acked=" << input.acked_state << " confirmed=" << input.confirmed_state
+                << " shelved=" << input.shelved << " branch=" << input.branch_id_present
+                << " prev=" << static_cast<int>(prev_status) << " action=" << static_cast<int>(outcome.action)
+                << std::endl;
+    }
     it->second.last_status = outcome.next_status;
     runtime_snapshot = it->second;
 
@@ -436,8 +456,10 @@ void OpcuaPoller::on_event(const AlarmEventConfig & cfg, const std::vector<opcua
       std::lock_guard cb_lock(event_alarm_callback_mutex_);
       cb_copy = event_alarm_callback_;
     }
-    std::cerr << "[opcua_poller] dispatching action=" << static_cast<int>(delivery.action)
-              << " cb_set=" << (cb_copy ? 1 : 0) << std::endl;
+    if (opcua_trace_enabled()) {
+      std::cerr << "[opcua_poller] dispatching action=" << static_cast<int>(delivery.action)
+                << " cb_set=" << (cb_copy ? 1 : 0) << std::endl;
+    }
     if (cb_copy) {
       cb_copy(delivery);
     }

src/ros2_medkit_plugins/ros2_medkit_opcua/test/fixtures/test_alarm_server/run_ctest.py

@@ -21,9 +21,13 @@
 against it via ``asyncua``, then terminates the binary cleanly.
 
 Skips (exits 77 - the CTest convention for a skipped test) when
-``asyncua`` is not importable, so contributors who only iterate on the
-plugin sources do not need a Python pip install. CI installs ``asyncua``
-in the integration job and observes the test as a real pass / fail.
+``asyncua`` is not importable AND the env var
+``ROS2_MEDKIT_OPCUA_SMOKE_REQUIRE`` is unset / 0. Contributors iterating
+on the plugin sources can keep the skip; CI sets the env var so a
+missing ``asyncua`` becomes a hard failure (exit 1) instead of a silent
+skip. Closes the regression gap Copilot flagged on PR #387 where a CI
+step that drops the ``asyncua`` install would silently mask smoke
+regressions.
 """
 
 import os
@@ -94,6 +98,16 @@ def main():
     try:
         import asyncua  # noqa: F401  pylint: disable=unused-import,import-outside-toplevel
     except ImportError:
+        # Default: skip with CTest convention (77) so local dev iteration
+        # without pip install just passes the build.
+        # CI sets ROS2_MEDKIT_OPCUA_SMOKE_REQUIRE=1 to turn the skip into
+        # a hard failure - so a CI job that loses its asyncua install
+        # cannot silently bypass the smoke check.
+        require = os.environ.get('ROS2_MEDKIT_OPCUA_SMOKE_REQUIRE', '0')
+        if require not in ('', '0'):
+            print('asyncua not installed and ROS2_MEDKIT_OPCUA_SMOKE_REQUIRE set - failing',
+                  file=sys.stderr)
+            return 1
         print('asyncua not installed - skipping smoke test (CTest skip code)')
         return CTEST_SKIP
 

src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_alarm_state_machine.cpp

@@ -223,7 +223,10 @@ TEST(AlarmStateMachineTest, DisabledClearsHealedAlarm) {
   EXPECT_EQ(out.action, AlarmAction::ClearFault);
 }
 
-TEST(AlarmStateMachineTest, DisabledNoOpWhenAlreadyCleared) {
+TEST(AlarmStateMachineTest, DisabledTransitionsClearedToSuppressedNoOp) {
+  // Disabled-while-Cleared: status DOES change (Cleared -> Suppressed) but
+  // no callback fires (NoOp action). Naming reflects both halves so a
+  // future reader does not misread "NoOp" as "no transition".
   AlarmEventInput in;
   in.enabled_state = false;
   auto out = AlarmStateMachine::compute(SovdAlarmStatus::Cleared, in);