fix(aggregation): hierarchical/leaf Component routing with provenance and warnings

[bburda]

Summary

Make the aggregation layer treat Components with the same symmetry as Areas, so collision-merged peer Components work correctly for every deployment shape the project supports (single peer, multi-peer, hierarchical parent, daisy chain).

• EntityMerger::merge_components now records a provisional routing entry on collision. Every Component ID refers to at most one ECU when it is a leaf, and the peer owns the runtime state.
• A new pure classify_component_routing free function (aggregation/classification.hpp) runs after all peers have been merged. A Component is classified as a hierarchical parent if any other Component in the merged set references it via parent_component_id; those are removed from the routing table and served locally from the merged cache. Leaves stay routed to the owning peer.
• Multi-peer leaf collisions are reported as structured LeafCollisionWarning objects. AggregationManager surfaces them on MergedPeerResult.leaf_warnings, writes an RCLCPP_WARN line listing every claimant, and exposes the list via a new /health.warnings array. Routing falls back to last-writer-wins because rejection does not fix a deployment anomaly.
• Every merged entity (Area, Component, App, Function) now carries an optional x-medkit.contributors list populated during merge (local seeded before the peer loop, peer:<name> appended by EntityMerger with de-duplication). Clients can distinguish local-only, peer-only, and merged views without relying on the single-valued source field. In daisy-chain topologies each hop surfaces only its direct upstream by design.
• Pre-existing doc comment referencing a third-party commercial product was stripped from sovd_service_interface.hpp.
• Aggregation design doc now documents the symmetric classification, the multi-peer warning behaviour, and the contributors provenance, with an updated PlantUML merge diagram plus a new post-merge classification diagram.

SOVD API impact

• No changes to SOVD-defined request paths, response schemas, or error codes.
• Additive x-medkit extensions only: new optional x-medkit.contributors field on entity responses and a new optional warnings array on /health. Both are emitted only when non-empty / when aggregation is active, so existing clients ignoring unknown fields are unaffected.

---

Issue

• closes [BUG] Collision-merged peer components return empty runtime data on primary #373

---

Type

• Bug fix
• New feature or tests (classification seam, /health.warnings, contributors, daisy-chain integration test)
• Breaking change
• Documentation only (design doc updates)

---

Testing

All local runs green:

• ./scripts/test.sh unit --packages-select ros2_medkit_gateway - 1976 tests, 0 failures. Includes:
  • 6 new AggregationClassification tests covering leaf vs hierarchical, single/multi-peer, subcomponents, and warning emission.
  • 4 new EntityMerger contributor tests (local/peer append, dedup, app-prefix-only-peer).
  • Updated merged_components_route_to_peer plus the synthetic-collision test from the previous iteration.
• ./scripts/test.sh test_peer_aggregation --packages-select ros2_medkit_integration_tests - prior integration coverage still green, 82 tests.
• ./scripts/test.sh test_daisy_chain_aggregation --packages-select ros2_medkit_integration_tests - new integration test with 3 gateways in isolated DDS domains (primary -> peer_B -> peer_C). Verifies hierarchical parent served locally, 1-hop and 2-hop leaf forwarding, contributors, and empty /health.warnings.
• ./scripts/test.sh lint --packages-select ros2_medkit_gateway - clean.
• Sphinx docs build with no new warnings.

---

Checklist

• Breaking changes are clearly described (there are none; only additive x-medkit extensions)
• Tests were added or updated if needed
• Docs were updated if behavior or public API changed

[Copilot]

Pull request overview

Fixes aggregation routing so that when a peer Component collides (same ID) with a locally-present Component and gets merged, sub-resource requests are forwarded to the peer that owns the runtime state—preventing empty local responses on endpoints like /logs, /hosts, /data, and /operations.

Changes:

• Add routing table entry for collision-merged Components in EntityMerger::merge_components.
• Update/extend EntityMerger unit tests to assert merged Components route to the peer (including a hybrid synthetic-collision scenario).
• Refresh aggregation design documentation and remove an unrelated third-party product reference from a plugin doc comment.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/ros2_medkit_gateway/src/aggregation/entity_merger.cpp	Route collision-merged Component IDs to the peer in the routing table.
src/ros2_medkit_gateway/test/test_entity_merger.cpp	Update routing-table expectations and add a hybrid synthetic-collision test.
src/ros2_medkit_gateway/design/aggregation.rst	Align merge/routing documentation with ECU-ownership routing behavior for Components.
src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/src/sovd_service_interface.hpp	Remove unrelated third-party reference from the class doc comment.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.

[Copilot]

The RESOURCE_LOCK is likely not being applied because the integration test targets are named from get_filename_component(... NAME_WE) on *.test.py files, which yields names like test_peer_aggregation.test, not test_peer_aggregation. As written, set_tests_properties(test_peer_aggregation ...) won’t match any test, so multi-gateway tests can still run concurrently and collide on the shared secondary DDS domains (230–232). Update the list to use the actual CTest target names (e.g. test_peer_aggregation.test, test_cross_ecu_fanout.test, test_daisy_chain_aggregation.test) or change the test_name derivation to strip both .test and .py consistently (and then use those names here).

[Copilot]

XMedkit::contributors() re-implements the same ordering logic as sorted_contributors() in discovery/models/common.hpp. Since list responses use sorted_contributors() while detail handlers use XMedkit::contributors(), keeping two independent implementations risks the ordering drifting and producing inconsistent API output over time. Consider reusing a single helper/comparator for both paths (e.g. have XMedkit::contributors() call sorted_contributors() or move the sort routine to a shared utility header used by both).

[mfaferek93]

Major - no validation on parent_component_id. This insert fires unconditionally for any non-empty parent ID. Three malformed inputs produce silent misbehavior (round-2 traced end-to-end):

1. Nonexistent parent (child.parent_component_id = "ghost"): "ghost" lands in hierarchical_parents set, gets excluded from routing_table (line 45-47). No Component object exists. Client GET /components/ghost → 404 via validate_entity_for_route (handler_context.cpp:254-267). The child itself is correctly routed, but the parent reference is a dangling pointer in the response.

2. Cycle A↔B (A.parent = B, B.parent = A): both IDs enter hierarchical_parents, neither enters routing_table. Both served from local merged cache (both exist there), never forwarded to their owning peer. Semantically wrong: if peer_b owns B, client reading /components/B hits the local stub instead of the real peer data.

3. Self-parent (A.parent = A): A inserted as its own hierarchical parent, excluded from routing, served locally. Breaks the invariant that hierarchical parents aggregate their children.

Pre-classification sanitization also absent at peer_client.cpp:180 (JSON deserialization) and entity_merger.cpp. Zero guard anywhere.

Suggested fix:

std::unordered_set<std::string> known_ids;
for (const auto & comp : merged_components) known_ids.insert(comp.id);

for (const auto & comp : merged_components) {
  const auto & p = comp.parent_component_id;
  if (p.empty()) continue;
  if (p == comp.id) {
    RCLCPP_WARN(..., "Component '%s' declares itself as parent, ignoring", p.c_str());
    continue;
  }
  if (known_ids.count(p) == 0u) {
    RCLCPP_WARN(..., "Component '%s' parent_component_id='%s' not in merged set", comp.id.c_str(), p.c_str());
    continue;
  }
  hierarchical_parents.insert(p);
}
// BFS/DFS cycle detection pass here.

Plus unit tests: parent_references_nonexistent_component, two_way_parent_cycle, self_parent.

[mfaferek93]

Major - routing destination flips silently across merge snapshots under peer health churn.

peer_component_claims is built by iterating futures (line 422 context below) in insertion order of the healthy-peer snapshot at line 371-374. is_healthy() filter silently drops failing peers. The "last writer" in classification.cpp:46 (result.routing_table[id] = pc.peer_name;) is whichever peer sits last in this vector.

Concrete scenario (round-2 traced):

• Merge N: peers [peer_b, peer_c] both claim ecu-shared. Last writer = peer_c. /health.warnings surfaces both claimants. Routing destination = peer_c.
• Merge N+1: peer_c fails health check, dropped from snapshot. peer_component_claims = [peer_b]. Last writer = peer_b. Only 1 claimant = no collision = no warning. Routing destination silently flipped from peer_c to peer_b.
• Operator sees /health go from "collision warning present" to "warnings: []" with no signal that routing destination changed.

This is worse than pure nondeterminism: the warning mechanism itself can hide the flip.

Three options:

1. Sticky routing: remember last-chosen peer across merges, only flip if that peer becomes unhealthy.
2. Deterministic ordering: std::sort(peer_component_claims.begin(), ..., by peer_name) before classification so the "last writer" is reproducible.
3. Emit routing-change event: new warning code WARN_ROUTING_DESTINATION_CHANGED whenever a routing entry for a given entity_id differs from the previous merge's entry.

At minimum, update design/aggregation.rst:291 to call out cross-snapshot instability and the warning-disappearance edge case so operators can plan around it.

[mfaferek93]

Minor - peer names leak on unauthenticated endpoint. Round-2 verified: auth.enabled defaults to false at gateway_node.cpp:169. Anyone reachable to /health enumerates every peer name in the deployment. Even with auth enabled, /health is in VIEWER/OPERATOR/CONFIGURATOR roles so any authenticated user sees the names.

Fine in controlled LAN deployments, leaks topology in shared-infra / multi-tenant setups. Since design intent is "operator-visible", that's OK - but please document explicitly in docs/api/rest.rst so operators know peer names on this endpoint are public-by-default.

If you want to gate it: wrap the warnings array in the same auth config that gates admin-adjacent info, while leaving the top-level status/uptime unauthenticated for classical health probes.

[mfaferek93]

Minor - no schema version for the warnings array. Codes are stable strings (good), but when the second code ships, clients have no contract to feature-detect "supports new_code_X" without string-matching. capabilities.aggregation is a boolean — too coarse.

Suggested: add response["warning_schema_version"] = 1; in health_handlers.cpp, bump on every new code added, document in warning_codes.rst. Typed clients (MCP, Web UI, Foxglove) can then make the "known codes" set explicit and log+degrade on unknown codes per version.

[mfaferek93]

Minor - collision test is unidirectional. Input is hardcoded [peer_b, peer_c] at line 184-185, asserted routing destination is peer_c (last). Flipping input to [peer_c, peer_b] should route to peer_b. That flip case is not tested, so a regression that breaks last-writer semantics (e.g., accidentally sorting claims by peer_name before classification) wouldn't be caught.

Add a parametrized or second test:

TEST(AggregationClassification, leaf_collision_routing_respects_input_order) {
  // [peer_c, peer_b] → routing must go to peer_b
  ...
}

Also missing from this file (round-2 confirmed all 6 test names): two_way_parent_cycle, self_parent_ignored, parent_references_nonexistent_component, empty_merged_set, four_level_nesting. See companion Major comment on classification.cpp:29.

[mfaferek93]

Minor - no end-to-end negative collision scenario in daisy-chain tests. All 10 test methods are positive paths. robot-x collision is intentional hierarchical merge (not an error case), and line 275 asserts body['warnings'] == [] for healthy chain.

Recommend a dedicated test: configure peer_b AND peer_c to both claim a new leaf ecu-shared (no parent_component_id), verify from primary:

1. /components/ecu-shared returns 200 and routes somewhere (either peer_b or peer_c).
2. /health.warnings contains a leaf_id_collision entry listing both peer_b and peer_c under peer_names.
3. x-medkit.contributors for the routed response reflects the resolving peer (not both).

This exercises the actual wire-level warning path under the real daisy-chain topology that unit tests can't simulate. Also covers the nondeterminism concern on aggregation_manager.cpp:419 via a follow-up test that kills one of the colliding peers and re-asserts warnings disappearance + routing flip (per the Major on that line).

[mfaferek93]

Minor - contributor presentation order not asserted on the user-visible path. This test verifies dedup in the internal contributors_ vector, which is correct. But clients actually see the output of XMedkit::contributors() in x_medkit.cpp:110-128, which sorts ["local", peers alphabetical].

Suggest one additional test that exercises the sorted output: inputs arriving in reverse order (e.g., ["peer:zulu", "peer:alpha", "local"]) should serialize as ["local", "peer:alpha", "peer:zulu"]. Otherwise a regression flipping the sort direction wouldn't be caught by unit tests — only by an integration test that happens to inspect the exact contributor order.