diff --git a/docs/tutorials/manifest-discovery.rst b/docs/tutorials/manifest-discovery.rst index 8da9b532..b98dd796 100644 --- a/docs/tutorials/manifest-discovery.rst +++ b/docs/tutorials/manifest-discovery.rst @@ -224,6 +224,9 @@ After merging, the **RuntimeLinker** binds manifest apps to running ROS 2 nodes: 3. **Linking**: For each manifest app, checks ``ros_binding`` configuration 4. **Binding**: If match found, copies runtime resources (topics, services, actions) 5. **Status**: Apps with matched nodes are marked ``is_online: true`` +6. **Suppression**: Heuristic entities that duplicate manifest-covered namespaces + (components/areas) or linked app IDs (apps) are removed. Gap-fill apps in + uncovered namespaces survive. Merge Report ~~~~~~~~~~~~ diff --git a/src/ros2_medkit_gateway/design/index.rst b/src/ros2_medkit_gateway/design/index.rst index 7731e5db..dd04637e 100644 --- a/src/ros2_medkit_gateway/design/index.rst +++ b/src/ros2_medkit_gateway/design/index.rst @@ -410,6 +410,8 @@ Main Components - Executes all layers, collects entities by ID, and merges them per-field-group - Each layer declares a ``MergePolicy`` per ``FieldGroup``: AUTHORITATIVE (wins), ENRICHMENT (fills empty), FALLBACK (last resort) - Runs ``RuntimeLinker`` post-merge to bind manifest apps to live ROS 2 nodes + - Suppresses runtime-origin entities that duplicate manifest entities: components/areas + by namespace match, apps by ID match (gap-fill apps in uncovered namespaces survive) - Produces a ``MergeReport`` with conflict diagnostics, enrichment counts, and ID collision detection **Built-in Layers:** diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/discovery/merge_types.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/discovery/merge_types.hpp index c7d7daee..6d4039ab 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/discovery/merge_types.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/discovery/merge_types.hpp @@ -151,5 +151,15 @@ struct GapFillConfig { std::vector namespace_blacklist; }; +/** + * @brief Check if an entity source is from runtime discovery (eligible for suppression) + * + * Runtime sources (heuristic, topic, synthetic) are created by the runtime discovery + * layer and may duplicate manifest entities. Manifest and plugin sources are preserved. + */ +inline bool is_runtime_source(const std::string & source) { + return source == "heuristic" || source == "topic" || source == "synthetic"; +} + } // namespace discovery } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp b/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp index accad1a0..b97f7c9d 100644 --- a/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp +++ b/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp @@ -14,6 +14,8 @@ #include "ros2_medkit_gateway/discovery/manifest/runtime_linker.hpp" +#include "ros2_medkit_gateway/discovery/merge_types.hpp" + #include namespace ros2_medkit_gateway { @@ -190,8 +192,7 @@ LinkingResult RuntimeLinker::link(const std::vector & manifest_apps, const } auto it = std::remove_if(result.linked_apps.begin(), result.linked_apps.end(), [&](const App & app) { - // Only suppress known runtime sources - preserve manifest and plugin entities - if (app.source != "heuristic" && app.source != "topic" && app.source != "synthetic") { + if (!is_runtime_source(app.source)) { return false; } if (!app.bound_fqn.has_value()) { diff --git a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp index c2e1eb45..51c96749 100644 --- a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp @@ -483,7 +483,19 @@ MergeResult MergePipeline::execute() { linking_result_ = linker_->get_last_result(); result.linking_result = linking_result_; - // Suppress runtime-origin components/areas that duplicate manifest entities (#307) + // Suppress runtime-origin entities that duplicate manifest entities (#307) + // In hybrid mode with a manifest, the manifest is the source of truth for entity + // structure. Heuristic entities from runtime discovery should be suppressed when + // their namespace is covered by manifest entities or linked apps. + // + // Suppression strategy (intentionally asymmetric): + // - Components/Areas: suppressed by NAMESPACE match. If any manifest app is linked + // in a namespace (including "/"), all heuristic components/areas in that namespace + // are removed. This means the manifest "takes over" the namespace entirely. + // - Apps: suppressed by ID match. Only heuristic apps whose ID matches a linked + // manifest app are removed. Gap-fill apps (new nodes not in manifest) survive + // regardless of namespace, since they fill intentional manifest gaps. + // Build set of namespaces covered by linked manifest apps std::set linked_namespaces; for (const auto & [fqn, app_id] : linking_result_.node_to_app) { @@ -495,8 +507,10 @@ MergeResult MergePipeline::execute() { auto last_slash = clean_fqn.rfind('/'); if (last_slash != std::string::npos && last_slash > 0) { linked_namespaces.insert(clean_fqn.substr(0, last_slash)); + } else if (last_slash == 0) { + // Root namespace node (e.g., /fault_manager) - include "/" as covered + linked_namespaces.insert("/"); } - // Skip root namespace "/" - too broad, would suppress all root-namespace entities } // Also track manifest component/area namespaces directly @@ -513,10 +527,35 @@ MergeResult MergePipeline::execute() { } } + // Remove heuristic apps that were merged into manifest apps (same ID after merge). + // These are runtime duplicates of linked manifest entities. Gap-fill apps (new + // heuristic apps in any namespace) survive - they fill manifest gaps intentionally. + std::set manifest_app_ids; + for (const auto & app : result.apps) { + if (app.source == "manifest") { + manifest_app_ids.insert(app.id); + } + } + // node_to_app values are always manifest app IDs (RuntimeLinker maps FQN -> manifest ID), + // so linked_app_ids == manifest_app_ids in practice. We merge both defensively in case + // the linker is extended to produce non-manifest IDs in the future. + std::set linked_app_ids(manifest_app_ids); + for (const auto & [fqn, app_id] : linking_result_.node_to_app) { + linked_app_ids.insert(app_id); + } + auto app_it = std::remove_if(result.apps.begin(), result.apps.end(), [&](const App & app) { + if (!is_runtime_source(app.source)) { + return false; + } + // Keep gap-fill apps (not linked to any manifest entity) + // Suppress only if this app's ID matches a linked manifest app + return linked_app_ids.count(app.id) > 0; + }); + result.apps.erase(app_it, result.apps.end()); + // Remove runtime components whose namespace is covered auto comp_it = std::remove_if(result.components.begin(), result.components.end(), [&](const Component & comp) { - // Only suppress known runtime sources - preserve manifest and plugin entities - if (comp.source != "heuristic" && comp.source != "topic" && comp.source != "synthetic") { + if (!is_runtime_source(comp.source)) { return false; } return manifest_comp_ns.count(comp.namespace_path) > 0 || linked_namespaces.count(comp.namespace_path) > 0; @@ -525,8 +564,7 @@ MergeResult MergePipeline::execute() { // Remove runtime areas whose namespace is covered auto area_it = std::remove_if(result.areas.begin(), result.areas.end(), [&](const Area & area) { - // Only suppress known runtime sources - preserve manifest and plugin entities - if (area.source != "heuristic" && area.source != "topic" && area.source != "synthetic") { + if (!is_runtime_source(area.source)) { return false; } return manifest_area_ns.count(area.namespace_path) > 0 || linked_namespaces.count(area.namespace_path) > 0; diff --git a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp index 70d0d12c..c2cdaa28 100644 --- a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp @@ -1588,10 +1588,10 @@ TEST_F(MergePipelineTest, AppExternalField_EnrichmentDoesNotStickyTrue) { EXPECT_FALSE(result.apps[0].external); } -TEST_F(MergePipelineTest, SuppressDoesNotAffectRootNamespaceEntities) { - // A manifest app bound to a root-level node (/root_node) should NOT - // suppress runtime components with empty namespace_path. - // This tests the last_slash > 0 guard in merge_pipeline.cpp. +TEST_F(MergePipelineTest, SuppressDoesNotAffectEmptyNamespaceEntities) { + // A manifest app bound to a root-level node (/root_node) adds "/" to + // linked_namespaces. But a runtime component with empty namespace_path "" + // (distinct from "/") should NOT be suppressed - empty namespace != root. // Manifest app with ros_binding to a root-level node App manifest_app = make_app("root_app", "some_comp"); @@ -1634,9 +1634,8 @@ TEST_F(MergePipelineTest, SuppressDoesNotAffectRootNamespaceEntities) { pipeline_.set_linker(std::make_unique(nullptr), manifest_config); auto result = pipeline_.execute(); - // The runtime component with empty namespace should NOT be suppressed. - // The last_slash > 0 guard prevents "/" FQNs from adding empty string - // to linked_namespaces, which would incorrectly match all root-namespace entities. + // The runtime component with empty namespace_path should NOT be suppressed. + // linked_namespaces contains "/" (from the root-level node), but "" != "/". bool found_root_comp = false; for (const auto & c : result.components) { if (c.id == "root_comp") { @@ -1669,3 +1668,208 @@ TEST_F(MergePipelineTest, AddLayerAfterExecuteDoesNotCrash) { auto result2 = pipeline_.execute(); EXPECT_EQ(result2.apps.size(), 2u); } + +// Regression: root-namespace linked nodes must suppress heuristic entities (#307) +TEST_F(MergePipelineTest, SuppressRootNamespaceHeuristicComponents) { + // Manifest component in root namespace + Component manifest_comp = make_component("fault-manager", "", "/"); + manifest_comp.source = "manifest"; + + // Runtime heuristic component also in root namespace (different ID) + Component runtime_comp = make_component("fault_manager", "", "/"); + runtime_comp.source = "heuristic"; + + // Manifest app with ros_binding to root-namespace node + App manifest_app = make_app("medkit-fault-manager", "fault-manager"); + manifest_app.source = "manifest"; + App::RosBinding binding; + binding.node_name = "fault_manager"; + binding.namespace_pattern = "/"; + binding.topic_namespace = ""; + manifest_app.ros_binding = binding; + + // Runtime app (node) that linker will match + App runtime_node; + runtime_node.id = "fault_manager"; + runtime_node.name = "fault_manager"; + runtime_node.source = "heuristic"; + runtime_node.is_online = true; + runtime_node.bound_fqn = "/fault_manager"; + + LayerOutput manifest_out; + manifest_out.components.push_back(manifest_comp); + manifest_out.apps.push_back(manifest_app); + + LayerOutput runtime_out; + runtime_out.components.push_back(runtime_comp); + runtime_out.apps.push_back(runtime_node); + + pipeline_.add_layer(std::make_unique( + "manifest", manifest_out, + std::unordered_map{{FieldGroup::IDENTITY, MergePolicy::AUTHORITATIVE}, + {FieldGroup::STATUS, MergePolicy::FALLBACK}})); + pipeline_.add_layer(std::make_unique( + "runtime", runtime_out, + std::unordered_map{{FieldGroup::STATUS, MergePolicy::AUTHORITATIVE}})); + + ManifestConfig manifest_config; + pipeline_.set_linker(std::make_unique(nullptr), manifest_config); + + auto result = pipeline_.execute(); + // Only manifest component should remain; root-namespace heuristic suppressed + ASSERT_EQ(result.components.size(), 1u); + EXPECT_EQ(result.components[0].id, "fault-manager"); + EXPECT_EQ(result.components[0].source, "manifest"); +} + +// Regression: root-namespace heuristic areas must be suppressed (#307) +TEST_F(MergePipelineTest, SuppressRootNamespaceHeuristicAreas) { + // Manifest area in /sensors + Area manifest_area; + manifest_area.id = "sensors"; + manifest_area.name = "Sensors"; + manifest_area.namespace_path = "/sensors"; + manifest_area.source = "manifest"; + + // Heuristic "root" area with namespace_path "/" - should be suppressed + Area root_area; + root_area.id = "root"; + root_area.name = "root"; + root_area.namespace_path = "/"; + root_area.source = "heuristic"; + + // Manifest app with ros_binding - root-namespace node links "/" into linked_namespaces + App manifest_app = make_app("fault-mgr", "fault-comp"); + manifest_app.source = "manifest"; + App::RosBinding binding; + binding.node_name = "fault_manager"; + binding.namespace_pattern = "/"; + binding.topic_namespace = ""; + manifest_app.ros_binding = binding; + + App runtime_node; + runtime_node.id = "fault_manager"; + runtime_node.name = "fault_manager"; + runtime_node.source = "heuristic"; + runtime_node.is_online = true; + runtime_node.bound_fqn = "/fault_manager"; + + LayerOutput manifest_out; + manifest_out.areas.push_back(manifest_area); + manifest_out.apps.push_back(manifest_app); + + LayerOutput runtime_out; + runtime_out.areas.push_back(root_area); + runtime_out.apps.push_back(runtime_node); + + pipeline_.add_layer(std::make_unique( + "manifest", manifest_out, + std::unordered_map{{FieldGroup::IDENTITY, MergePolicy::AUTHORITATIVE}})); + pipeline_.add_layer(std::make_unique( + "runtime", runtime_out, + std::unordered_map{{FieldGroup::STATUS, MergePolicy::AUTHORITATIVE}})); + + ManifestConfig manifest_config; + pipeline_.set_linker(std::make_unique(nullptr), manifest_config); + + auto result = pipeline_.execute(); + // Only manifest area should remain; root heuristic area suppressed + ASSERT_EQ(result.areas.size(), 1u); + EXPECT_EQ(result.areas[0].id, "sensors"); + EXPECT_EQ(result.areas[0].source, "manifest"); +} + +// Heuristic apps in covered namespaces are suppressed; uncovered survive (#307) +TEST_F(MergePipelineTest, SuppressHeuristicAppsInCoveredNamespace) { + // Manifest app in /sensors namespace + App manifest_app = make_app("lidar-sim", "lidar-unit"); + manifest_app.source = "manifest"; + App::RosBinding binding; + binding.node_name = "lidar_sim"; + binding.namespace_pattern = "/sensors"; + binding.topic_namespace = ""; + manifest_app.ros_binding = binding; + + // Runtime app that links to manifest (same namespace - /sensors) + App linked_runtime; + linked_runtime.id = "lidar_sim"; + linked_runtime.name = "lidar_sim"; + linked_runtime.source = "heuristic"; + linked_runtime.is_online = true; + linked_runtime.bound_fqn = "/sensors/lidar_sim"; + + // Heuristic app in root namespace - root is covered (linked via /fault_manager) + App root_heuristic; + root_heuristic.id = "_param_client_node"; + root_heuristic.name = "_param_client_node"; + root_heuristic.source = "heuristic"; + root_heuristic.is_online = true; + root_heuristic.bound_fqn = "/_param_client_node"; + + // Manifest app in root namespace (creates "/" in linked_namespaces) + App root_manifest = make_app("fault-mgr", "fault-comp"); + root_manifest.source = "manifest"; + App::RosBinding root_binding; + root_binding.node_name = "fault_manager"; + root_binding.namespace_pattern = "/"; + root_binding.topic_namespace = ""; + root_manifest.ros_binding = root_binding; + + App root_runtime; + root_runtime.id = "fault_manager"; + root_runtime.name = "fault_manager"; + root_runtime.source = "heuristic"; + root_runtime.is_online = true; + root_runtime.bound_fqn = "/fault_manager"; + + // Gap-fill app in uncovered namespace (/actuators) - should survive + App gapfill_app; + gapfill_app.id = "motor_ctrl"; + gapfill_app.name = "motor_ctrl"; + gapfill_app.source = "heuristic"; + gapfill_app.is_online = true; + gapfill_app.bound_fqn = "/actuators/motor_ctrl"; + + LayerOutput manifest_out; + manifest_out.apps.push_back(manifest_app); + manifest_out.apps.push_back(root_manifest); + + LayerOutput runtime_out; + runtime_out.apps.push_back(linked_runtime); + runtime_out.apps.push_back(root_heuristic); + runtime_out.apps.push_back(root_runtime); + runtime_out.apps.push_back(gapfill_app); + + pipeline_.add_layer(std::make_unique( + "manifest", manifest_out, + std::unordered_map{{FieldGroup::IDENTITY, MergePolicy::AUTHORITATIVE}})); + pipeline_.add_layer(std::make_unique( + "runtime", runtime_out, + std::unordered_map{{FieldGroup::STATUS, MergePolicy::AUTHORITATIVE}})); + + ManifestConfig manifest_config; + pipeline_.set_linker(std::make_unique(nullptr), manifest_config); + + auto result = pipeline_.execute(); + // 2 manifest apps + 1 gap-fill (/actuators) + 1 orphan (_param_client_node, not in linked_app_ids) + // Removed: linked_runtime (FQN-deduped with lidar-sim), root_runtime (FQN-deduped with fault-mgr) + ASSERT_EQ(result.apps.size(), 4u); + std::set app_ids; + for (const auto & a : result.apps) { + app_ids.insert(a.id); + } + EXPECT_TRUE(app_ids.count("lidar-sim")) << "manifest app should survive"; + EXPECT_TRUE(app_ids.count("fault-mgr")) << "manifest app should survive"; + EXPECT_TRUE(app_ids.count("motor_ctrl")) << "gap-fill app in uncovered namespace should survive"; + EXPECT_TRUE(app_ids.count("_param_client_node")) + << "unmanifested heuristic app should survive (ID-based suppression only targets linked IDs)"; + + // Verify source provenance on surviving apps + for (const auto & a : result.apps) { + if (a.id == "lidar-sim" || a.id == "fault-mgr") { + EXPECT_EQ(a.source, "manifest") << a.id << " should be manifest-sourced"; + } else { + EXPECT_EQ(a.source, "heuristic") << a.id << " should be heuristic-sourced"; + } + } +}