fix: suppress root-namespace heuristic entities and orphan apps#312
fix: suppress root-namespace heuristic entities and orphan apps#312
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes remaining hybrid-mode entity suppression gaps in the discovery merge pipeline to avoid emitting duplicate root-namespace heuristic entities and orphan runtime apps when a manifest is present (regression/continuation of #307).
Changes:
- Treat linked root-namespace nodes (e.g.,
/fault_manager) as covering namespace/during suppression. - Add post-link suppression of orphan runtime apps in hybrid mode when a manifest is present.
- Add unit tests covering root-namespace component/area suppression and orphan app suppression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp |
Extends post-link suppression logic to include root namespace coverage and removes orphan runtime apps when a manifest exists. |
src/ros2_medkit_gateway/test/test_merge_pipeline.cpp |
Adds regression tests for root-namespace suppression and orphan runtime app suppression. |
| // Build set of manifest app IDs for orphan suppression | ||
| std::set<std::string> manifest_app_ids; | ||
| for (const auto & app : result.apps) { | ||
| if (app.source == "manifest") { | ||
| manifest_app_ids.insert(app.id); | ||
| } | ||
| } | ||
| bool has_manifest = !manifest_app_ids.empty(); | ||
|
|
||
| // Remove heuristic apps that are orphans (not linked to any manifest entity) | ||
| // In hybrid mode with a manifest, orphan heuristic apps are noise | ||
| if (has_manifest) { | ||
| auto app_it = std::remove_if(result.apps.begin(), result.apps.end(), [&](const App & app) { | ||
| if (app.source != "heuristic" && app.source != "topic" && app.source != "synthetic") { | ||
| return false; | ||
| } | ||
| // Suppress if this heuristic app was not linked to a manifest app | ||
| return manifest_app_ids.find(app.id) == manifest_app_ids.end(); | ||
| }); | ||
| result.apps.erase(app_it, result.apps.end()); | ||
| } |
There was a problem hiding this comment.
The orphan-app suppression ignores ManifestConfig::unmanifested_nodes (documented as warn/include_as_orphan = include orphans) and currently drops all runtime-origin apps whenever any manifest apps exist. Also, using manifest_app_ids.find(app.id) can fail when an orphan runtime node happens to share an ID with a manifest app but wasn’t linked (different FQN), causing the orphan to be kept. Consider gating suppression on manifest_config_.unmanifested_nodes == IGNORE and detecting orphans via linking_result_.node_to_app / linking_result_.orphan_nodes using app.bound_fqn rather than comparing IDs.
| ManifestConfig manifest_config; | ||
| pipeline_.set_linker(std::make_unique<RuntimeLinker>(nullptr), manifest_config); | ||
|
|
There was a problem hiding this comment.
This test relies on the default ManifestConfig (which is unmanifested_nodes: warn) but asserts that orphan runtime apps are suppressed. If suppression is meant to be policy-controlled (per manifest schema/docs), set manifest_config.unmanifested_nodes = IGNORE here, and/or add coverage that warn keeps orphans.
bburda
force-pushed
the
fix/suppress-root-namespace-heuristic-entities
branch
from
be5fdb9 to
1d33b71
Compare
Two remaining suppression gaps in hybrid mode merge pipeline: 1. Root-namespace linked nodes (e.g., /fault_manager) were excluded from linked_namespaces because last_slash == 0 was skipped. Now "/" is added to linked_namespaces for root-namespace FQNs. 2. Heuristic apps that were merged into manifest apps (same ID after linking) are now suppressed. Gap-fill apps (new heuristic entities NOT linked to manifest) are preserved regardless of namespace. Three regression tests added: - SuppressRootNamespaceHeuristicComponents - SuppressRootNamespaceHeuristicAreas - SuppressHeuristicAppsInCoveredNamespace
bburda
force-pushed
the
fix/suppress-root-namespace-heuristic-entities
branch
from
1d33b71 to
3e2c6ec
Compare
- Extract is_runtime_source() helper to merge_types.hpp, replacing duplicated "heuristic/topic/synthetic" predicates in merge_pipeline.cpp (3 sites) and runtime_linker.cpp (1 site) - Document asymmetric suppression strategy: components/areas use namespace matching, apps use ID matching (gap-fill apps must survive) - Clarify linked_app_ids defensive merge comment - Fix stale test comments in SuppressDoesNotAffectRootNamespaceEntities (rename to SuppressDoesNotAffectEmptyNamespaceEntities, update docs)
…tions - Add source field verification on surviving apps in SuppressHeuristicAppsInCoveredNamespace test (manifest apps must have source="manifest", gap-fill apps must have source="heuristic") - Add source verification to SuppressRootNamespaceHeuristicAreas test - Document post-merge suppression step in design doc (index.rst) - Document suppression as step 6 in tutorial pipeline sequence
Pull Request
Summary
Fix remaining entity suppression gaps in hybrid mode merge pipeline. After PR #311, gateway no longer crashes but still emits duplicate entities from root namespace and orphan nodes.
Three issues fixed:
/fault_manager) -linked_namespacesnow includes"/"for root-namespace FQNs (previously skipped due tolast_slash == 0guard)_param_client_node) - in hybrid mode with manifest, heuristic apps not present in manifest are suppressedrootarea) - now covered by fix Add devcontainer for ROS 2 development #1Before: sensor demo had 6 areas (expected 4), 10 components (expected 8), 9 apps (expected 8)
After: exact manifest counts
Issue
Type
Testing
SuppressRootNamespaceHeuristicComponents,SuppressRootNamespaceHeuristicAreas,SuppressOrphanHeuristicAppsChecklist