fix(log_bridge): address PR review feedback

Stable FNV-1a fault_code, uniform node_source_id, sanitized prefix, LRU-bounded reporters, null guard. Cooldown is now ERROR/FATAL-only and keyed by severity so a WARN cannot drop an ERROR escalation. Unique test domain range, docs-build wiring, and a launch_testing integration test for the live /rosout path.

Author: mfaferek93

docs/Doxyfile

@@ -14,6 +14,10 @@ INPUT                  = ../src/ros2_medkit_gateway/include \
                          ../src/ros2_medkit_fault_reporter/src \
                          ../src/ros2_medkit_diagnostic_bridge/include \
                          ../src/ros2_medkit_diagnostic_bridge/src \
+                         ../src/ros2_medkit_log_bridge/include \
+                         ../src/ros2_medkit_log_bridge/src \
+                         ../src/ros2_medkit_action_status_bridge/include \
+                         ../src/ros2_medkit_action_status_bridge/src \
                          ../src/ros2_medkit_serialization/include \
                          ../src/ros2_medkit_serialization/src \
                          ../src/ros2_medkit_msgs

docs/api/cpp.rst

@@ -83,6 +83,22 @@ Bridge node that converts ROS 2 /diagnostics messages to FaultManager faults.
 .. doxygenclass:: ros2_medkit_diagnostic_bridge::DiagnosticBridgeNode
    :members:
 
+ros2_medkit_log_bridge
+----------------------
+
+Bridge node that promotes ROS 2 /rosout log entries to FaultManager faults.
+
+.. doxygenclass:: ros2_medkit_log_bridge::LogBridgeNode
+   :members:
+
+ros2_medkit_action_status_bridge
+--------------------------------
+
+Bridge node that turns terminal ROS 2 action goal states into FaultManager faults.
+
+.. doxygenclass:: ros2_medkit_action_status_bridge::ActionStatusBridgeNode
+   :members:
+
 ros2_medkit_serialization
 -------------------------
 

docs/changelog.rst

@@ -14,6 +14,10 @@ This page aggregates changelogs from all ros2_medkit packages.
 
 .. include:: ../src/ros2_medkit_diagnostic_bridge/CHANGELOG.rst
 
+.. include:: ../src/ros2_medkit_log_bridge/CHANGELOG.rst
+
+.. include:: ../src/ros2_medkit_action_status_bridge/CHANGELOG.rst
+
 .. include:: ../src/ros2_medkit_serialization/CHANGELOG.rst
 
 .. include:: ../src/ros2_medkit_msgs/CHANGELOG.rst

docs/introduction.rst

@@ -59,6 +59,12 @@ ros2_medkit consists of several ROS 2 packages:
 **ros2_medkit_diagnostic_bridge**
    Bridge node that converts standard ROS 2 ``/diagnostics`` messages to fault manager faults.
 
+**ros2_medkit_log_bridge**
+   Bridge node that promotes ROS 2 ``/rosout`` log entries to fault manager faults.
+
+**ros2_medkit_action_status_bridge**
+   Bridge node that turns terminal ROS 2 action goal states (aborted) into fault manager faults.
+
 **ros2_medkit_msgs**
    Message and service definitions for fault management.
 

src/ros2_medkit_cmake/cmake/ROS2MedkitTestDomain.cmake

@@ -27,7 +27,9 @@
 #   ros2_medkit_topic_beacon:       110 - 119 (10 slots)
 #   ros2_medkit_graph_provider:     120 - 129 (10 slots)
 #   ros2_medkit_linux_introspection: 130 - 139 (10 slots)
-#   ros2_medkit_integration_tests:  140 - 219 (80 slots)
+#   ros2_medkit_integration_tests:  140 - 209 (70 slots)
+#   ros2_medkit_log_bridge:         210 - 214 (5 slots, carved from integration_tests)
+#   ros2_medkit_action_status_bridge: 215 - 219 (5 slots, carved from integration_tests)
 #   ros2_medkit_opcua:              220 - 229 (10 slots, carved from integration_tests)
 #   multi-domain tests (secondary): 230 - 232 (3 slots, reserved for peer_aggregation etc.)
 #

src/ros2_medkit_integration_tests/CMakeLists.txt

@@ -110,7 +110,7 @@ if(BUILD_TESTING)
   set(_INTEG_PORT 9100)
 
   include(ROS2MedkitTestDomain)
-  medkit_init_test_domains(START 140 END 229)
+  medkit_init_test_domains(START 140 END 209)
 
   # Feature tests (atomic, independent, 120s timeout)
   file(GLOB FEATURE_TESTS test/features/*.test.py)

src/ros2_medkit_log_bridge/CMakeLists.txt

@@ -88,6 +88,7 @@ ament_export_dependencies(rclcpp rcl_interfaces ros2_medkit_msgs ros2_medkit_fau
 if(BUILD_TESTING)
   find_package(ament_lint_auto REQUIRED)
   find_package(ament_cmake_gtest REQUIRED)
+  find_package(launch_testing_ament_cmake REQUIRED)
 
   set(ament_cmake_clang_format_CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../.clang-format")
   list(APPEND AMENT_LINT_AUTO_EXCLUDE ament_cmake_uncrustify ament_cmake_cpplint ament_cmake_clang_tidy)
@@ -96,7 +97,7 @@ if(BUILD_TESTING)
   ros2_medkit_clang_tidy()
 
   include(ROS2MedkitTestDomain)
-  medkit_init_test_domains(START 90 END 99)
+  medkit_init_test_domains(START 210 END 214)
 
   ament_add_gtest(test_log_bridge test/test_log_bridge.cpp)
   target_link_libraries(test_log_bridge log_bridge_lib)
@@ -108,6 +109,16 @@ if(BUILD_TESTING)
     target_link_options(test_log_bridge PRIVATE --coverage)
   endif()
 
+  # Integration test (launch_testing): fault_manager + bridge + synthetic /rosout
+  install(DIRECTORY test
+    DESTINATION share/${PROJECT_NAME}
+  )
+  add_launch_test(
+    test/test_integration.test.py
+    TARGET test_integration
+    TIMEOUT 90
+  )
+
   ros2_medkit_relax_vendor_warnings()
 endif()
 

src/ros2_medkit_log_bridge/README.md

@@ -21,22 +21,51 @@ above a severity floor to the FaultManager:
 | ERROR (40) | `SEVERITY_ERROR` |
 | FATAL (50) | `SEVERITY_CRITICAL` |
 
-- `source_id` of each fault is the originating node (the `Log.name` field), via
-  a per-node `FaultReporter`, so faults attribute correctly and each node gets
-  its own local debounce.
-- `fault_code` is auto-generated as `<PREFIX>_<NODE>_<HASH>`, where the hash is
-  taken over a normalized message template (numbers / hex / paths stripped) so
-  the same logical message maps to the same code across occurrences.
-
-## WARN as PREFAILED
-
-The bridge forwards WARN immediately. Whether a WARN shows as `PREFAILED`
-(suspected, kept out of the confirmed-fault list) or `CONFIRMED` is decided by
-the FaultManager's `confirmation_threshold`, not the bridge. For the
-visible-but-quiet behaviour, launch the FaultManager with
-`confirmation_threshold:=-2` or lower (or an entity threshold for `LOG_*`
-codes). With the shipped default (`-1`), every WARN confirms on first
-occurrence.
+- `source_id` of each fault is the originating node's fully-qualified name. It
+  is derived from `Log.name` by taking the first dotted segment (a `Log.name`
+  may carry a sub-logger suffix, e.g. `controller_manager.resource_manager`, and
+  node names cannot contain `.`) and prefixing `/`, giving e.g.
+  `/controller_manager`. The gateway discovers entities by node FQN, so this is
+  the form that lets a fault (and its snapshots / rosbag) associate with the
+  entity in the SOVD tree. Each node gets its own per-node `FaultReporter` and
+  therefore its own client-side debounce.
+- `fault_code` is auto-generated as `<PREFIX>_<NODE>_<HASH>`. `<HASH>` is a fixed
+  FNV-1a 32-bit digest (8 lowercase hex) of a normalized message template
+  (numbers / hex / paths stripped, isolated single-letter tokens dropped) so the
+  same logical message maps to the same code across occurrences. `<NODE>` is the
+  upper-snake of `source_id`. The 8-hex hash is never truncated; if the 64-char
+  cap is hit the node part is trimmed instead.
+
+> Namespaced-node limitation: `Log.name` encodes a node's namespace with the same
+> `.` separator as a sub-logger suffix, so the two are indistinguishable from the
+> string alone. `source_id` takes the first dotted segment, which is right for a
+> non-namespaced node with a sub-logger but collapses a namespaced node
+> (`robot1.planner_server` -> `/robot1`) to its namespace, so same-named nodes in
+> different namespaces share one code. Multi-robot fleets typically isolate robots
+> by `ROS_DOMAIN_ID` (one gateway per robot, federated by peer aggregation), which
+> sidesteps this.
+
+## Forwarding, the LocalFilter, and confirmation
+
+Two independent debounces sit between a log line and a confirmed fault:
+
+1. Per-node `FaultReporter` `LocalFilter` (client-side). WARN is held until
+   `default_threshold` (3) occurrences within `default_window_sec` (10s).
+   ERROR/FATAL have severity `>= bypass_severity` (2) and bypass the filter,
+   forwarding immediately.
+2. Bridge `report_cooldown_sec` cooldown, applied only to `ERROR`/`FATAL` (the
+   levels that bypass the LocalFilter). It forwards the first occurrence of a
+   `(fault_code, severity)` immediately and suppresses that same pair for
+   `report_cooldown_sec` (default 5s, `0.0` disables), bounding a flood. `WARN`
+   is never cooled here (that would starve its LocalFilter threshold counting),
+   and keying on severity means a `WARN` never suppresses a same-message `ERROR`
+   escalation.
+
+Whether a forwarded fault then shows as `PREFAILED` (suspected) or `CONFIRMED`
+is a separate, gateway-side decision driven by the FaultManager's
+`confirmation_threshold` - not by this bridge and not by the client-side
+LocalFilter. For visible-but-quiet WARNs, launch the FaultManager with a low
+`confirmation_threshold` (or an entity threshold for `LOG_*` codes).
 
 ## Hard limitations (by construction)
 
@@ -59,7 +88,14 @@ ros2 launch ros2_medkit_log_bridge log_bridge.launch.py
 | Param | Default | Meaning |
 |-------|---------|---------|
 | `rosout_topic` | `/rosout` | log topic to subscribe |
-| `severity_floor` | `30` (WARN) | minimum level promoted; raise to `40` on chatty / constrained targets |
-| `code_prefix` | `LOG` | prefix for generated fault codes |
-| `exclude_nodes` | `[]` | node-name substrings to skip |
-| `include_only_nodes` | `[]` | if set, only promote these nodes |
+| `severity_floor` | `30` (WARN) | minimum level promoted; raise to `40` on chatty / constrained targets. Clamped to `[0, 50]` at load (a value out of range is corrected with a warning) |
+| `code_prefix` | `LOG` | prefix for generated fault codes; normalized to `[A-Z0-9_]` at load |
+| `exclude_nodes` | `[]` | node-FQN substrings to skip |
+| `include_only_nodes` | `[]` | if set, only promote nodes whose FQN matches |
+| `max_tracked_nodes` | `512` | cap on per-node reporters; least-recently-used nodes evicted past this |
+| `report_cooldown_sec` | `5.0` | per-fault_code forward debounce; `0.0` disables |
+
+`exclude_nodes` / `include_only_nodes` match as **unanchored substrings**
+against the node FQN: `planner` matches `/planner_server` and
+`/robot1/planner_server`. Use a longer, more specific substring (e.g.
+`/planner_server`) to avoid accidental matches.

src/ros2_medkit_log_bridge/config/log_bridge.yaml

@@ -8,7 +8,14 @@ log_bridge:
     severity_floor: 30
     # Prefix for auto-generated fault codes (<PREFIX>_<NODE>_<HASH>).
     code_prefix: "LOG"
-    # Originating-node name substrings to skip (e.g. noisy debug nodes).
+    # Originating-node FQN substrings to skip (e.g. noisy debug nodes).
     exclude_nodes: []
     # If non-empty, ONLY promote logs from nodes matching these substrings.
     include_only_nodes: []
+    # Cap on per-node FaultReporters; least-recently-used nodes are evicted
+    # past this to bound memory under transient-node churn.
+    max_tracked_nodes: 512
+    # Per-fault_code forward debounce (seconds). First occurrence of a code is
+    # forwarded immediately; the same code within the window is suppressed.
+    # 0.0 disables. Tames ERROR/FATAL floods, which bypass the per-node filter.
+    report_cooldown_sec: 5.0

src/ros2_medkit_log_bridge/include/ros2_medkit_log_bridge/log_bridge_node.hpp

@@ -14,10 +14,11 @@
 
 #pragma once
 
-#include <map>
+#include <list>
 #include <memory>
 #include <mutex>
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #include "rclcpp/rclcpp.hpp"
@@ -30,11 +31,11 @@ namespace ros2_medkit_log_bridge {
 ///
 /// Subscribes to /rosout (rcl_interfaces/msg/Log) and forwards entries at or
 /// above a configurable severity floor to the FaultManager, attributing each
-/// fault to the originating node (the Log.name field) via a per-source
+/// fault to the originating node's fully-qualified name via a per-source
 /// FaultReporter. Drop-in compat adapter, same category as
 /// ros2_medkit_diagnostic_bridge: native FaultReporter instrumentation stays
 /// the canonical path; this bridge is the fallback for nodes that only log.
-/// Level mapping and the WARN-as-PREFAILED caveat are documented in README.md.
+/// Level mapping and the WARN/LocalFilter caveat are documented in README.md.
 ///
 /// Hard limitation by construction: only sees rclcpp logs that reach /rosout
 /// from a still-alive node. Console-only loggers and crash-before-flush are out
@@ -47,19 +48,20 @@ class LogBridgeNode : public rclcpp::Node {
   /// Returns false when the level is below the floor / not promotable.
   static bool map_level_to_severity(uint8_t log_level, uint8_t severity_floor, uint8_t * severity_out);
 
-  /// Auto-generate a stable fault code from the originating node name and the
+  /// Auto-generate a stable fault code from the originating node's FQN and the
   /// log message. Numbers/hex/paths in the message are normalized away so the
   /// same logical message maps to the same code across occurrences.
   /// Format: <PREFIX>_<NODE>_<HASH>, clamped to medkit's [A-Z0-9_] / 64-char rule.
-  std::string generate_fault_code(const std::string & node_name, const std::string & message) const;
+  std::string generate_fault_code(const std::string & source_id, const std::string & message) const;
 
   /// Normalize a log message into a stable template (lowercased, digit/hex/path
-  /// runs stripped, whitespace collapsed). Exposed for unit testing.
+  /// runs stripped, whitespace collapsed, isolated single-letter tokens dropped).
+  /// Exposed for unit testing.
   static std::string normalize_message(const std::string & message);
 
   /// Whether a given originating node should be promoted, honouring the
   /// include/exclude lists. Exposed for unit testing.
-  bool node_is_eligible(const std::string & node_name) const;
+  bool node_is_eligible(const std::string & source_id) const;
 
   /// Map an rcl_interfaces/msg/Log.name (a logger name, e.g. "bt_navigator" or
   /// "controller_manager.resource_manager") to the originating node's
@@ -69,29 +71,55 @@ class LogBridgeNode : public rclcpp::Node {
   /// the entity in the SOVD tree. Exposed for unit testing.
   static std::string node_source_id(const std::string & log_name);
 
- private:
-  void log_callback(const rcl_interfaces::msg::Log::ConstSharedPtr & msg);
+  /// FNV-1a 32-bit hash, fixed spec, emitted as 8 lowercase hex chars. Exposed
+  /// for unit testing so a known input asserts a known constant.
+  static std::string fnv1a_hex(const std::string & in);
+
+  /// Whether a (fault_code, severity) may be forwarded now under the cooldown
+  /// (first occurrence passes; same code+severity within report_cooldown_sec is
+  /// suppressed; 0.0 disables). Keyed by severity so a WARN never suppresses a
+  /// same-message ERROR escalation. Exposed for unit testing.
+  bool cooldown_allows(const std::string & fault_code, uint8_t severity, rclcpp::Time now);
 
   /// Fetch (or lazily create) the per-source FaultReporter for an originating
   /// node, so the fault's source_id is the node that logged, not the bridge.
-  ros2_medkit_fault_reporter::FaultReporter * reporter_for(const std::string & node_name);
+  /// Bounded by max_tracked_nodes_ with LRU eviction. Exposed for unit testing.
+  ros2_medkit_fault_reporter::FaultReporter * reporter_for(const std::string & source_id);
+
+  /// Number of currently tracked per-node reporters. Exposed for unit testing.
+  size_t tracked_reporter_count();
+
+ private:
+  void log_callback(const rcl_interfaces::msg::Log::ConstSharedPtr & msg);
 
   void load_parameters();
 
   static std::string to_upper_snake(const std::string & in, size_t max_len);
 
   rclcpp::Subscription<rcl_interfaces::msg::Log>::SharedPtr log_sub_;
 
-  // One FaultReporter per originating node (correct source_id + own LocalFilter).
-  std::map<std::string, std::unique_ptr<ros2_medkit_fault_reporter::FaultReporter>> reporters_;
+  // One FaultReporter per originating node (correct source_id + own LocalFilter),
+  // LRU-ordered: front() is the least-recently-used entry.
+  struct ReporterEntry {
+    std::string source_id;
+    std::unique_ptr<ros2_medkit_fault_reporter::FaultReporter> reporter;
+  };
+  std::list<ReporterEntry> reporters_lru_;
+  std::unordered_map<std::string, std::list<ReporterEntry>::iterator> reporters_;
   std::mutex reporters_mutex_;
 
+  // Per-fault_code forward cooldown: last time a code was forwarded.
+  std::unordered_map<std::string, rclcpp::Time> last_forward_;
+  std::mutex cooldown_mutex_;
+
   // Configuration
   std::string rosout_topic_;
   uint8_t severity_floor_;
   std::string code_prefix_;
   std::vector<std::string> exclude_nodes_;
   std::vector<std::string> include_only_nodes_;
+  int max_tracked_nodes_;
+  double report_cooldown_sec_;
   std::string own_node_name_;
 };