From dad5d176d2e717f339fac0feaaf36adb7de18362 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sat, 11 Jul 2026 17:11:34 +0200 Subject: [PATCH 1/3] fix(gateway): preserve plugin App external flag through hybrid merge A manifest entry that declares an app id without the external key carries the bool default false. Under hybrid discovery the manifest layer wins the METADATA group, so that default erased the external=true classification a protocol plugin set on the same app. Without the flag the app lost its bare-id fault scope and its faults silently vanished from Function and Component fault rollups. manifest_only mode was not affected because it bypasses the merge pipeline. Merge external monotonically instead: once any layer classifies an app as external, a layer that does not know the classification cannot clear it. A plain bool cannot distinguish "not declared" from "declared false", so first-set-wins and authoritative-wins semantics both let a stub entry erase a real classification. Update the merge semantics pins to the new behavior, add the missing Function-hosts-external-app fault-scope regression, and document the exception in the discovery options and manifest schema docs. Fixes #517 --- docs/config/discovery-options.rst | 9 ++ docs/config/manifest-schema.rst | 4 +- .../src/discovery/merge_pipeline.cpp | 11 +-- .../test/test_handler_context.cpp | 19 ++++ .../test/test_merge_pipeline.cpp | 87 ++++++++++++++----- 5 files changed, 103 insertions(+), 27 deletions(-) diff --git a/docs/config/discovery-options.rst b/docs/config/discovery-options.rst index c8862fdba..1de8aabf6 100644 --- a/docs/config/discovery-options.rst +++ b/docs/config/discovery-options.rst @@ -175,6 +175,15 @@ Each layer declares a policy per field-group: - Manifest: ``authoritative`` for identity/hierarchy/metadata, ``enrichment`` for live_data, ``fallback`` for status - Runtime: ``authoritative`` for live_data/status, ``enrichment`` for metadata, ``fallback`` for identity/hierarchy +.. note:: + + The app ``external`` flag is an exception to the policy table: it merges + monotonically. Once any layer classifies an app as external (e.g. a + protocol plugin introspecting a PLC), the classification is kept even if a + higher-priority layer carries the default ``false`` - a manifest entry that + omits ``external:`` cannot clear it. This keeps external apps owning their + faults (bare-id fault scope) in hybrid mode. + Override per-layer policies in ``gateway_params.yaml``. Empty string means "use layer default". Policy values are **case-sensitive** and must be lowercase (``authoritative``, ``enrichment``, ``fallback``): diff --git a/docs/config/manifest-schema.rst b/docs/config/manifest-schema.rst index 0c40eedc5..70d7c44f3 100644 --- a/docs/config/manifest-schema.rst +++ b/docs/config/manifest-schema.rst @@ -550,7 +550,9 @@ Fields * - ``external`` - boolean - No - - True if not a ROS node (default: false) + - True if not a ROS node (default: false). In hybrid mode the flag merges + monotonically: omitting it does not clear an ``external`` classification + contributed by another discovery layer (e.g. a protocol plugin). ros_binding Fields ~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp index 84f9d7e21..6405dccde 100644 --- a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp @@ -278,11 +278,12 @@ void apply_field_group_merge(Entity & target, const Entity & source, FieldGroup case FieldGroup::METADATA: merge_scalar(target.source, source.source, res.scalar); merge_optional(target.ros_binding, source.ros_binding, res.scalar); - // Use scalar semantics (not OR) - external is a classification, not a status flag - if (res.scalar == MergeWinner::SOURCE) { - target.external = source.external; - } - // TARGET and BOTH: keep target value (no OR semantics) + // `external` is a classification a layer either knows (true) or cannot + // express (bool default false == unset). No layer can un-classify, so + // once any layer marks the app external it stays - a manifest stub's + // default false must not erase a plugin's introspected classification, + // or the app silently drops out of every fault rollup (#517). + target.external = target.external || source.external; break; } } else if constexpr (std::is_same_v) { diff --git a/src/ros2_medkit_gateway/test/test_handler_context.cpp b/src/ros2_medkit_gateway/test/test_handler_context.cpp index 422b9578c..52c36e307 100644 --- a/src/ros2_medkit_gateway/test/test_handler_context.cpp +++ b/src/ros2_medkit_gateway/test/test_handler_context.cpp @@ -1699,6 +1699,25 @@ TEST(ResolveEntitySourceFqnsTest, ComponentHostingExternalAppOwnsItsFaults) { EXPECT_EQ(fqns, std::set{"process"}); } +TEST(ResolveEntitySourceFqnsTest, FunctionHostingExternalAppOwnsItsFaults) { + // GET /functions//faults where the manifest Function's hosted_by lists an + // external app directly (App-host, not Component-host). The Function's scope + // must include the app's bare id alongside the FQNs of its ROS-bound hosts, + // otherwise faults the external app reported vanish from the Function rollup. + ThreadSafeEntityCache cache; + cache.update_apps( + {make_external_app("process", "s7_1500"), make_owned_app("planner", "nav_comp", "planner", "/perception/nav")}); + Function f; + f.id = "level_control"; + f.hosts = {"process", "planner"}; + cache.update_functions({f}); + + auto fqns = + HandlerContext::resolve_entity_source_fqns(cache, make_entity_info(EntityType::FUNCTION, "level_control")); + std::set expected{"process", "/perception/nav/planner"}; + EXPECT_EQ(fqns, expected); +} + int main(int argc, char ** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp index b78ae4d15..bf30b175a 100644 --- a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp @@ -993,6 +993,47 @@ TEST_F(MergePipelineTest, ThreeLayerMerge_PerFieldGroupOwnerTracking) { EXPECT_EQ(result.apps[0].bound_fqn, "/nav/controller"); } +// --- App METADATA merge: external classification --- + +// A protocol plugin introspects an external asset (e.g. a PLC over OPC UA) and +// emits its App with external=true and no ROS binding. In hybrid mode the +// manifest may declare the same App id as a bare stub (so a Function's +// hosted_by can reference it) without an `external:` key, which parses to the +// bool default false. `external` has no unset state, so the manifest's +// AUTHORITATIVE METADATA must not let that default erase the plugin's +// classification - collect_app_fqn() grants an empty-fqn App its bare-id fault +// scope only when external is true, so dropping the flag silently empties the +// App's fault rollup on every route that aggregates it. +TEST_F(MergePipelineTest, PluginExternalClassificationSurvivesManifestMetadataMerge) { + // Manifest stub: declares the App id, no external key -> default false. + App manifest_app = make_app("plc_process", "s7_1500"); + manifest_app.source = "manifest"; + + // Plugin introspection: same id, classified external, no ROS binding. + App plugin_app = make_app("plc_process", "s7_1500"); + plugin_app.external = true; + plugin_app.source = "opcua"; + + LayerOutput manifest_out, plugin_out; + manifest_out.apps.push_back(manifest_app); + plugin_out.apps.push_back(plugin_app); + + // Policies mirror the real layer defaults: ManifestLayer METADATA=AUTHORITATIVE, + // PluginLayer METADATA=ENRICHMENT. + pipeline_.add_layer(std::make_unique( + "manifest", manifest_out, + std::unordered_map{{FieldGroup::METADATA, MergePolicy::AUTHORITATIVE}})); + pipeline_.add_layer(std::make_unique( + "plugin", plugin_out, + std::unordered_map{{FieldGroup::METADATA, MergePolicy::ENRICHMENT}})); + + auto result = pipeline_.execute(); + ASSERT_EQ(result.apps.size(), 1u); + EXPECT_TRUE(result.apps[0].external) << "hybrid merge dropped the plugin's external classification"; + // The manifest stays the entity's structural owner. + EXPECT_EQ(result.apps[0].source, "manifest"); +} + // --- GapFillConfig namespace filtering --- // These tests verify the namespace matching semantics used by RuntimeLayer. // Since filter_by_namespace is internal to runtime_layer.cpp, we replicate the @@ -1414,30 +1455,31 @@ TEST_F(MergePipelineTest, RuntimeComponentInUncoveredNamespaceNotSuppressed) { EXPECT_TRUE(found_motor) << "Runtime component in uncovered namespace should not be suppressed"; } -TEST_F(MergePipelineTest, AppExternalField_ManifestAuthoritativeWinsOverRuntime) { - // Manifest layer: METADATA=AUTHORITATIVE, sets external=false (internal node) - App manifest_app = make_app("lidar_proc", "sensor_comp"); - manifest_app.external = false; +TEST_F(MergePipelineTest, AppExternalField_HigherPrioritySourceDefaultFalseCannotClear) { + // SOURCE-winner branch of the external merge: the base entity comes from a + // plugin (ENRICHMENT) that classified the app external; a later layer wins + // METADATA with AUTHORITATIVE but carries only the bool default false. + // `external` is monotone (#517): a layer that does not know the + // classification cannot clear it, regardless of its merge priority. + App plugin_app = make_app("plc_process", "s7_1500"); + plugin_app.external = true; - // Runtime layer: METADATA=ENRICHMENT, claims external=true (heuristic) - App runtime_app = make_app("lidar_proc", "sensor_comp"); - runtime_app.external = true; + App manifest_app = make_app("plc_process", "s7_1500"); - LayerOutput manifest_out, runtime_out; + LayerOutput plugin_out, manifest_out; + plugin_out.apps.push_back(plugin_app); manifest_out.apps.push_back(manifest_app); - runtime_out.apps.push_back(runtime_app); + pipeline_.add_layer(std::make_unique( + "plugin", plugin_out, + std::unordered_map{{FieldGroup::METADATA, MergePolicy::ENRICHMENT}})); pipeline_.add_layer(std::make_unique( "manifest", manifest_out, std::unordered_map{{FieldGroup::METADATA, MergePolicy::AUTHORITATIVE}})); - pipeline_.add_layer(std::make_unique( - "runtime", runtime_out, - std::unordered_map{{FieldGroup::METADATA, MergePolicy::ENRICHMENT}})); auto result = pipeline_.execute(); ASSERT_EQ(result.apps.size(), 1u); - // Manifest AUTHORITATIVE METADATA wins: external must be false - EXPECT_FALSE(result.apps[0].external); + EXPECT_TRUE(result.apps[0].external); } // --- Area field group merge tests --- @@ -1630,12 +1672,15 @@ TEST_F(MergePipelineTest, RuntimeLayerProducesNoAreasOrComponents) { EXPECT_EQ(result.apps[0].id, "sensor_node"); } -// --- Regression for #260c: external field OR bug --- +// --- External classification is monotone across layers (#517) --- -// Regression for #260c: external with ENRICHMENT-vs-ENRICHMENT should NOT use OR. -// Both layers METADATA=ENRICHMENT. First layer sets external=false, second sets external=true. -// In the old bug (OR semantics) result would be true. Now first layer wins (value already set). -TEST_F(MergePipelineTest, AppExternalField_EnrichmentDoesNotStickyTrue) { +// BOTH-winner branch (ENRICHMENT vs ENRICHMENT): a later layer's external=true +// classification is adopted even though the first layer already carried the +// bool default false. This deliberately supersedes the earlier first-set-wins +// pin (#260c): with a plain bool, false is indistinguishable from "not +// declared", and first-set-wins let a stub entry erase a real classification, +// silently dropping the app from every fault rollup (#517). +TEST_F(MergePipelineTest, AppExternalField_EnrichmentClassificationIsSticky) { App layer1_app = make_app("controller", "nav_comp"); layer1_app.external = false; @@ -1653,8 +1698,8 @@ TEST_F(MergePipelineTest, AppExternalField_EnrichmentDoesNotStickyTrue) { auto result = pipeline_.execute(); ASSERT_EQ(result.apps.size(), 1u); - // First layer's value must win for ENRICHMENT (first-set-wins, not OR). - EXPECT_FALSE(result.apps[0].external); + // Once any layer classifies the app as external, the classification stays. + EXPECT_TRUE(result.apps[0].external); } TEST_F(MergePipelineTest, SuppressDoesNotAffectEmptyNamespaceEntities) { From d7413b269101f9cabe6852b9138e43fa731ad6d6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sat, 11 Jul 2026 17:53:27 +0200 Subject: [PATCH 2/3] feat(gateway): warn on manifest apps combining external and ros_binding external: true means "not a ROS node": the runtime linker skips the binding and, under hybrid discovery, the external classification is kept once any layer sets it. Declaring both on one app is contradictory and previously passed validation silently. New rule R013 raises a validation warning so the conflict is visible instead of one side being ignored. --- docs/config/manifest-schema.rst | 2 + .../discovery/manifest/manifest_validator.cpp | 11 +++++ .../test/test_manifest_validator.cpp | 48 ++++++++++++++++++- 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/docs/config/manifest-schema.rst b/docs/config/manifest-schema.rst index 70d7c44f3..90d2ff82a 100644 --- a/docs/config/manifest-schema.rst +++ b/docs/config/manifest-schema.rst @@ -553,6 +553,8 @@ Fields - True if not a ROS node (default: false). In hybrid mode the flag merges monotonically: omitting it does not clear an ``external`` classification contributed by another discovery layer (e.g. a protocol plugin). + Combining ``external: true`` with a ``ros_binding`` is contradictory and + raises validation warning R013 (the binding is ignored for linking). ros_binding Fields ~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp b/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp index 80df42ffa..57ea3fc71 100644 --- a/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp +++ b/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp @@ -186,6 +186,17 @@ void ManifestValidator::validate_ros_bindings(const Manifest & manifest, Validat for (const auto & app : manifest.apps) { if (app.ros_binding.has_value() && !app.ros_binding->is_empty()) { + // external means "not a ROS node": the runtime linker skips the binding + // and the hybrid merge keeps the external classification. Declaring both + // is contradictory - warn instead of silently ignoring one of the two. + if (app.external) { + result.add_warning("R013", + "App '" + app.id + + "' declares external: true together with a ros_binding; " + "the binding is ignored for linking", + "apps/" + app.id + "/ros_binding"); + } + std::string binding_key = app.ros_binding->node_name + "@" + app.ros_binding->namespace_pattern; // For topic namespace bindings diff --git a/src/ros2_medkit_gateway/test/test_manifest_validator.cpp b/src/ros2_medkit_gateway/test/test_manifest_validator.cpp index bb8d61def..1a01d08e7 100644 --- a/src/ros2_medkit_gateway/test/test_manifest_validator.cpp +++ b/src/ros2_medkit_gateway/test/test_manifest_validator.cpp @@ -16,7 +16,7 @@ * @file test_manifest_validator.cpp * @brief Unit tests for manifest validator * - * @verifies REQ_DISCOVERY_003 Manifest validation rules R001-R012 + * @verifies REQ_DISCOVERY_003 Manifest validation rules R001-R013 */ #include @@ -453,6 +453,52 @@ manifest_version: "1.0" EXPECT_TRUE(result.is_valid); } +// ============================================================================= +// R013: External App With ROS Binding +// ============================================================================= + +TEST_F(ManifestValidatorTest, R013_ExternalAppWithRosBindingWarns) { + // external: true means "not a ROS node": the linker skips the binding and, + // in hybrid mode, a plugin's external classification cannot be cleared by + // other layers. Declaring both is contradictory, so surface it instead of + // silently ignoring one of the two. + const std::string yaml = R"( +manifest_version: "1.0" +apps: + - id: "plc_process" + external: true + ros_binding: + node_name: "plc_node" + namespace: "/plant" +)"; + + auto manifest = parser_.parse_string(yaml); + auto result = validator_.validate(manifest); + + EXPECT_TRUE(result.is_valid); + ASSERT_TRUE(result.has_warnings()); + EXPECT_EQ(result.warnings[0].rule_id, "R013"); +} + +TEST_F(ManifestValidatorTest, R013_ExternalAppWithoutBindingNoWarning) { + const std::string yaml = R"( +manifest_version: "1.0" +apps: + - id: "plc_process" + external: true + - id: "ros_app" + ros_binding: + node_name: "node1" + namespace: "/ns" +)"; + + auto manifest = parser_.parse_string(yaml); + auto result = validator_.validate(manifest); + + EXPECT_TRUE(result.is_valid); + EXPECT_FALSE(result.has_warnings()); +} + // ============================================================================= // R011: Circular Dependency Detection // ============================================================================= From 7f52b568b518ea1848a2af595783a95b769e3167 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 12 Jul 2026 11:32:32 +0200 Subject: [PATCH 3/3] fix(gateway): make App external tri-state to preserve classification through hybrid merge Change App.external from bool to std::optional so discovery can tell "no layer classified this app" (nullopt) from "declared not external" (false). A plain bool collapsed both to false, letting a manifest stub erase a protocol plugin's introspected classification and drop the app from every fault rollup (#517). - merge_external gap-fills like merge_scalar: an omitted classification never shadows another layer's value, while an explicit value from a higher-priority layer wins, so an authoritative manifest external: false corrects a wrong plugin true. (Plain merge_optional would not: its TARGET branch is a no-op and would drop the plugin's value, since the manifest layer is the AUTHORITATIVE base for METADATA.) - Fault scope checks external before effective_fqn, so an external app declared with a stray ros_binding is scoped by its bare entity id instead of a dead derived FQN (the neighbor case, same vanishing-faults symptom as #517). - Manifest validation R013 skips the duplicate-binding bookkeeping for external apps, so their inert binding no longer trips a phantom R010 on a real app that reuses the node name. - Parser preserves omitted vs explicit false; discovery and manifest-schema docs describe the tri-state merge. Unit coverage for the new merge branches, the neighbor-case fault scope, the Area rollup and the R010 phantom-conflict fix; plus an end-to-end integration test that reports a fault under an external app's bare id and asserts it on the app, component, function and area rollup routes. Closes #517 --- docs/config/discovery-options.rst | 16 +- docs/config/manifest-schema.rst | 13 +- .../examples/external_app_fault_manifest.yaml | 63 +++++++ .../core/discovery/models/app.hpp | 6 +- .../discovery/manifest/manifest_validator.cpp | 14 +- .../src/core/discovery/models/app.cpp | 8 +- .../src/core/faults/fault_scope.cpp | 21 ++- .../discovery/manifest/manifest_parser.cpp | 7 +- .../src/discovery/manifest/runtime_linker.cpp | 2 +- .../src/discovery/merge_pipeline.cpp | 31 +++- .../test/test_handler_context.cpp | 40 +++++ .../test/test_manifest_parser.cpp | 38 +++- .../test/test_manifest_validator.cpp | 42 ++++- .../test/test_merge_pipeline.cpp | 86 +++++++--- .../test_external_app_fault_rollup.test.py | 162 ++++++++++++++++++ 15 files changed, 480 insertions(+), 69 deletions(-) create mode 100644 src/ros2_medkit_gateway/config/examples/external_app_fault_manifest.yaml create mode 100644 src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py diff --git a/docs/config/discovery-options.rst b/docs/config/discovery-options.rst index 1de8aabf6..a77ed6b7b 100644 --- a/docs/config/discovery-options.rst +++ b/docs/config/discovery-options.rst @@ -177,12 +177,16 @@ Each layer declares a policy per field-group: .. note:: - The app ``external`` flag is an exception to the policy table: it merges - monotonically. Once any layer classifies an app as external (e.g. a - protocol plugin introspecting a PLC), the classification is kept even if a - higher-priority layer carries the default ``false`` - a manifest entry that - omits ``external:`` cannot clear it. This keeps external apps owning their - faults (bare-id fault scope) in hybrid mode. + The app ``external`` classification is tri-state (unset / ``true`` / + ``false``), so it follows the policy table with one refinement: a layer that + does not classify the app (a manifest entry that omits ``external:``) never + clears or shadows a classification another layer contributed. A manifest stub + that omits ``external:`` therefore keeps a protocol plugin's introspected + ``external: true`` (so the app still owns its faults via bare-id fault scope + in hybrid mode). An *explicit* value is authoritative and resolves by normal + layer priority: an authoritative manifest ``external: false`` overrides a + plugin's ``external: true``. Only omission is a no-op; an explicit value is + never silently discarded. Override per-layer policies in ``gateway_params.yaml``. Empty string means "use layer default". Policy values are **case-sensitive** and must be lowercase diff --git a/docs/config/manifest-schema.rst b/docs/config/manifest-schema.rst index 90d2ff82a..fa29d85ad 100644 --- a/docs/config/manifest-schema.rst +++ b/docs/config/manifest-schema.rst @@ -550,11 +550,16 @@ Fields * - ``external`` - boolean - No - - True if not a ROS node (default: false). In hybrid mode the flag merges - monotonically: omitting it does not clear an ``external`` classification - contributed by another discovery layer (e.g. a protocol plugin). + - True if not a ROS node (treated as not external when omitted). The + classification is tri-state internally: **omitting** ``external:`` leaves + it unset, so in hybrid mode it does not clear an ``external`` + classification contributed by another discovery layer (e.g. a protocol + plugin) - the app keeps its bare-id fault scope. An **explicit** value is + authoritative and resolves by normal layer priority, so an authoritative + manifest ``external: false`` overrides a plugin's ``external: true``. Combining ``external: true`` with a ``ros_binding`` is contradictory and - raises validation warning R013 (the binding is ignored for linking). + raises validation warning R013 (the binding is ignored for linking and + fault scoping - the app is scoped by its entity id). ros_binding Fields ~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_gateway/config/examples/external_app_fault_manifest.yaml b/src/ros2_medkit_gateway/config/examples/external_app_fault_manifest.yaml new file mode 100644 index 000000000..a4283a9d3 --- /dev/null +++ b/src/ros2_medkit_gateway/config/examples/external_app_fault_manifest.yaml @@ -0,0 +1,63 @@ +# SOVD System Manifest: External-app fault-rollup fixture (#517) +# ============================================================== +# Minimal hybrid-mode manifest that models a non-ROS asset (a PLC bridged into +# SOVD by a protocol plugin) as an external App. The App has external: true and +# reports faults to the fault_manager under its own entity id rather than a ROS +# FQN. Fault-scope resolution must recognise that bare id on the App route and +# on every aggregate route that rolls it up (component, function, area) - #517 +# was the fault scope pointing at a live FQN instead of the bare id, which +# silently emptied those rollups. +# +# The App deliberately also carries a ros_binding (the #517 "neighbor" case): a +# manifest may declare a binding while a protocol plugin classifies the asset +# external. effective_fqn() then derives a live FQN "/plc/plc_process", so a +# fault scope that used the FQN would drop the app's faults (reported under the +# bare id). The external classification must win - the app is scoped by its +# entity id. This also raises validation warning R013 (external + ros_binding). +# +# Used by: test/features/test_external_app_fault_rollup.test.py + +manifest_version: "1.0" + +metadata: + name: "external-app-fault-rollup" + version: "1.0.0" + description: "External (non-ROS) app fault-rollup regression fixture (#517)" + +config: + unmanifested_nodes: warn + inherit_runtime_resources: true + +areas: + - id: plc-cell + name: "PLC Cell" + namespace: /plc_cell + description: "Industrial cell hosting a PLC bridged into SOVD" + +components: + - id: s7-plc + name: "Siemens S7 PLC" + type: "controller" + area: plc-cell + description: "Non-ROS PLC introspected by a protocol plugin" + +apps: + - id: plc-process + name: "PLC Process" + external: true + is_located_on: s7-plc + description: "Non-ROS process bridged into SOVD; reports faults under its entity id" + # Stray binding (#517 neighbor case): effective_fqn() would derive + # "/plc/plc_process", but the external classification must win so fault + # scope stays the bare id "plc-process". + ros_binding: + node_name: plc_process + namespace: /plc + +functions: + - id: level-control + name: "Level Control" + category: "control" + description: "Level control capability hosted by the PLC process" + hosted_by: + - plc-process diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/discovery/models/app.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/discovery/models/app.hpp index fcb82a433..995497c12 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/discovery/models/app.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/discovery/models/app.hpp @@ -91,7 +91,11 @@ struct App { // === Runtime state (populated after linking) === std::optional bound_fqn; ///< Bound ROS node FQN bool is_online{false}; ///< Whether the bound node is running - bool external{false}; ///< True if not a ROS node + /// Tri-state classification: nullopt = no layer classified this app, + /// true = not a ROS node (external asset), false = explicitly a ROS node. + /// A plain bool cannot distinguish "omitted" from "declared false", which let + /// a manifest stub erase a plugin's introspected classification (#517). + std::optional external; /// Get effective FQN: bound_fqn if available, otherwise derived from ros_binding. /// Returns empty string if neither is available. Used by handlers and samplers diff --git a/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp b/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp index 57ea3fc71..111a074c6 100644 --- a/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp +++ b/src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp @@ -187,14 +187,18 @@ void ManifestValidator::validate_ros_bindings(const Manifest & manifest, Validat for (const auto & app : manifest.apps) { if (app.ros_binding.has_value() && !app.ros_binding->is_empty()) { // external means "not a ROS node": the runtime linker skips the binding - // and the hybrid merge keeps the external classification. Declaring both - // is contradictory - warn instead of silently ignoring one of the two. - if (app.external) { + // and fault scoping uses the app's entity id, so the binding is inert. + // Declaring both is contradictory - warn, and skip the R010 bookkeeping + // entirely: an inert binding must not reserve a node name and trip a + // phantom duplicate error on a real ROS app that later reuses it. + if (app.external.value_or(false)) { result.add_warning("R013", "App '" + app.id + - "' declares external: true together with a ros_binding; " - "the binding is ignored for linking", + "' declares external: true together with a ros_binding; the binding " + "is ignored for linking and fault scoping (the app is scoped by its " + "entity id)", "apps/" + app.id + "/ros_binding"); + continue; } std::string binding_key = app.ros_binding->node_name + "@" + app.ros_binding->namespace_pattern; diff --git a/src/ros2_medkit_gateway/src/core/discovery/models/app.cpp b/src/ros2_medkit_gateway/src/core/discovery/models/app.cpp index 5eff4cd83..09c2efd56 100644 --- a/src/ros2_medkit_gateway/src/core/discovery/models/app.cpp +++ b/src/ros2_medkit_gateway/src/core/discovery/models/app.cpp @@ -48,8 +48,10 @@ json App::to_json() const { x_medkit["boundFqn"] = bound_fqn.value(); } x_medkit["isOnline"] = is_online; - if (external) { - x_medkit["external"] = external; + // Emit only when effectively external; omitted and explicit-false both leave + // the field absent, keeping the wire shape stable (absence == not external). + if (external.value_or(false)) { + x_medkit["external"] = true; } if (!original_id.empty()) { x_medkit["original_id"] = original_id; @@ -116,7 +118,7 @@ json App::to_capabilities(const std::string & base_url) const { j["operations"] = app_base + "/operations"; } // Always include configurations (parameters) for non-external apps - if (!external) { + if (!external.value_or(false)) { j["configurations"] = app_base + "/configurations"; } // Always include faults diff --git a/src/ros2_medkit_gateway/src/core/faults/fault_scope.cpp b/src/ros2_medkit_gateway/src/core/faults/fault_scope.cpp index f02a2daf0..9b2d20341 100644 --- a/src/ros2_medkit_gateway/src/core/faults/fault_scope.cpp +++ b/src/ros2_medkit_gateway/src/core/faults/fault_scope.cpp @@ -29,17 +29,20 @@ void collect_app_fqn(const ThreadSafeEntityCache & cache, const std::string & ap if (!app) { return; } + // External assets introspected by a protocol plugin (e.g. a PLC over OPC UA) + // report faults to the fault_manager under their own entity id, so that id is + // their sole fault-scope owner. This must be checked before effective_fqn(): + // a manifest may declare a stray ros_binding on an external app, and the + // derived FQN would otherwise shadow the bare id and drop its faults from + // every rollup (#517, neighbor case). + if (app->external.value_or(false)) { + out.insert(app_id); + return; + } auto fqn = app->effective_fqn(); if (fqn.empty()) { - // External assets introspected by a protocol plugin (e.g. a PLC over OPC - // UA) have no ROS binding, so effective_fqn() is empty. They report faults - // to the fault_manager under their own entity id, so that id is their sole - // fault-scope owner. Non-external apps that merely failed to bind stay - // skipped: granting an unbound ROS app its bare id would let it claim - // ownership of faults it never reported. - if (app->external) { - out.insert(app_id); - } + // A non-external app that merely failed to bind stays skipped: granting an + // unbound ROS app its bare id would let it claim faults it never reported. return; } out.insert(std::move(fqn)); diff --git a/src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp b/src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp index b6b01f9af..2a226c9df 100644 --- a/src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp +++ b/src/ros2_medkit_gateway/src/discovery/manifest/manifest_parser.cpp @@ -378,7 +378,12 @@ App ManifestParser::parse_app(const YAML::Node & node) const { app.component_id = get_string(node, "is_located_on"); app.depends_on = get_string_vector(node, "depends_on"); app.tags = get_string_vector(node, "tags"); - app.external = node["external"] ? node["external"].as() : false; + // Preserve the omitted-vs-explicit distinction: an absent `external:` key + // leaves nullopt (the manifest does not classify), so the hybrid merge cannot + // let a stub's default erase a plugin's introspected classification (#517). + if (node["external"]) { + app.external = node["external"].as(); + } app.source = "manifest"; // Parse ros_binding 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 84f42971f..255c036fc 100644 --- a/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp +++ b/src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp @@ -83,7 +83,7 @@ LinkingResult RuntimeLinker::link(const std::vector & manifest_apps, const linked_app.is_online = false; // External apps don't need ROS binding - if (manifest_app.external) { + if (manifest_app.external.value_or(false)) { result.linked_apps.push_back(linked_app); continue; } diff --git a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp index 6405dccde..d10bdb657 100644 --- a/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp @@ -169,6 +169,30 @@ void merge_optional(std::optional & target, const std::optional & source, } } +// Merge the tri-state `external` classification. Mirrors merge_scalar's +// gap-filling policy (nullopt == "empty"): a higher-priority layer that does not +// classify (nullopt) must never shadow a lower-priority layer's classification, +// so once any layer sets it the value survives (a manifest stub cannot hide a +// plugin's introspected external=true, #517). An explicit value from a strictly +// higher-priority layer (SOURCE) still wins, so an authoritative manifest +// `external: false` corrects a wrong plugin `true`. This is NOT merge_optional: +// merge_optional's TARGET branch is a no-op and would drop the plugin's value. +void merge_external(std::optional & target, const std::optional & source, MergeWinner winner) { + switch (winner) { + case MergeWinner::SOURCE: + if (source.has_value()) { + target = source; + } + break; + case MergeWinner::BOTH: + case MergeWinner::TARGET: + if (!target.has_value() && source.has_value()) { + target = source; + } + break; + } +} + void merge_topics(ComponentTopics & target, const ComponentTopics & source, MergeWinner winner) { merge_collection(target.publishes, source.publishes, winner); merge_collection(target.subscribes, source.subscribes, winner); @@ -278,12 +302,7 @@ void apply_field_group_merge(Entity & target, const Entity & source, FieldGroup case FieldGroup::METADATA: merge_scalar(target.source, source.source, res.scalar); merge_optional(target.ros_binding, source.ros_binding, res.scalar); - // `external` is a classification a layer either knows (true) or cannot - // express (bool default false == unset). No layer can un-classify, so - // once any layer marks the app external it stays - a manifest stub's - // default false must not erase a plugin's introspected classification, - // or the app silently drops out of every fault rollup (#517). - target.external = target.external || source.external; + merge_external(target.external, source.external, res.scalar); break; } } else if constexpr (std::is_same_v) { diff --git a/src/ros2_medkit_gateway/test/test_handler_context.cpp b/src/ros2_medkit_gateway/test/test_handler_context.cpp index 52c36e307..dadd49c5a 100644 --- a/src/ros2_medkit_gateway/test/test_handler_context.cpp +++ b/src/ros2_medkit_gateway/test/test_handler_context.cpp @@ -1718,6 +1718,46 @@ TEST(ResolveEntitySourceFqnsTest, FunctionHostingExternalAppOwnsItsFaults) { EXPECT_EQ(fqns, expected); } +TEST(ResolveEntitySourceFqnsTest, AreaHostingExternalAppOwnsItsFaults) { + // GET /areas//faults. The area's component hosts an external app (a PLC + // over OPC UA - the opcua plugin sets comp.area). The area walks the same gate + // as the component/function routes (collect_area_app_fqns -> collect_app_fqn), + // so its scope must include the external app's bare id or the app's faults + // vanish from the area rollup. + ThreadSafeEntityCache cache; + Component plc; + plc.id = "s7_1500"; + plc.area = "cell_a"; + cache.update_components({plc}); + cache.update_apps({make_external_app("process", "s7_1500")}); + + auto fqns = HandlerContext::resolve_entity_source_fqns(cache, make_entity_info(EntityType::AREA, "cell_a")); + EXPECT_EQ(fqns, std::set{"process"}); +} + +TEST(ResolveEntitySourceFqnsTest, ExternalAppWithStrayRosBindingOwnsFaultsByBareId) { + // The #517 "neighbor" case: a manifest declares the app with a real + // ros_binding (so effective_fqn() derives a live FQN) and a plugin classifies + // it external. If fault scope used the derived FQN, the app's faults (reported + // under its bare id) would vanish - the exact #517 symptom. The external + // classification must win: the scope is the bare id, never the FQN. + ThreadSafeEntityCache cache; + App a; + a.id = "process"; + a.name = "process"; + a.component_id = "s7_1500"; + a.external = true; + App::RosBinding binding; + binding.node_name = "process"; + binding.namespace_pattern = "/plc"; + a.ros_binding = binding; // stray binding: effective_fqn() would be "/plc/process" + cache.update_apps({a}); + + auto fqns = HandlerContext::resolve_entity_source_fqns(cache, make_entity_info(EntityType::APP, "process")); + // Bare id owns the faults; the derived FQN "/plc/process" must NOT appear. + EXPECT_EQ(fqns, std::set{"process"}); +} + int main(int argc, char ** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_gateway/test/test_manifest_parser.cpp b/src/ros2_medkit_gateway/test/test_manifest_parser.cpp index d9c239d35..18466196c 100644 --- a/src/ros2_medkit_gateway/test/test_manifest_parser.cpp +++ b/src/ros2_medkit_gateway/test/test_manifest_parser.cpp @@ -256,7 +256,43 @@ manifest_version: "1.0" auto manifest = parser_.parse_string(yaml); ASSERT_EQ(manifest.apps.size(), 1); - EXPECT_TRUE(manifest.apps[0].external); + ASSERT_TRUE(manifest.apps[0].external.has_value()); + EXPECT_TRUE(manifest.apps[0].external.value()); +} + +TEST_F(ManifestParserTest, ParseAppExternalOmittedStaysUnset) { + // Tri-state: an absent `external:` key must leave the classification unset + // (nullopt), not collapse to false - otherwise a manifest stub would erase a + // plugin's introspected classification in the hybrid merge (#517). + const std::string yaml = R"( +manifest_version: "1.0" +apps: + - id: "ros_node" + name: "ROS Node" +)"; + + auto manifest = parser_.parse_string(yaml); + + ASSERT_EQ(manifest.apps.size(), 1); + EXPECT_FALSE(manifest.apps[0].external.has_value()); +} + +TEST_F(ManifestParserTest, ParseAppExternalExplicitFalseIsPreserved) { + // An explicit `external: false` is authoritative and must be distinguishable + // from omission, so the merge can let it override a wrong plugin `true`. + const std::string yaml = R"( +manifest_version: "1.0" +apps: + - id: "ros_node" + name: "ROS Node" + external: false +)"; + + auto manifest = parser_.parse_string(yaml); + + ASSERT_EQ(manifest.apps.size(), 1); + ASSERT_TRUE(manifest.apps[0].external.has_value()); + EXPECT_FALSE(manifest.apps[0].external.value()); } TEST_F(ManifestParserTest, ParseFunctions) { diff --git a/src/ros2_medkit_gateway/test/test_manifest_validator.cpp b/src/ros2_medkit_gateway/test/test_manifest_validator.cpp index 1a01d08e7..4867eac96 100644 --- a/src/ros2_medkit_gateway/test/test_manifest_validator.cpp +++ b/src/ros2_medkit_gateway/test/test_manifest_validator.cpp @@ -458,10 +458,9 @@ manifest_version: "1.0" // ============================================================================= TEST_F(ManifestValidatorTest, R013_ExternalAppWithRosBindingWarns) { - // external: true means "not a ROS node": the linker skips the binding and, - // in hybrid mode, a plugin's external classification cannot be cleared by - // other layers. Declaring both is contradictory, so surface it instead of - // silently ignoring one of the two. + // external: true means "not a ROS node": the linker skips the binding and + // fault scoping uses the app's entity id, so the binding is inert. Declaring + // both is contradictory, so surface it instead of silently ignoring one. const std::string yaml = R"( manifest_version: "1.0" apps: @@ -499,6 +498,41 @@ manifest_version: "1.0" EXPECT_FALSE(result.has_warnings()); } +TEST_F(ManifestValidatorTest, R013_ExternalAppBindingDoesNotReserveNodeName) { + // An external app's ros_binding is inert (ignored for linking and fault + // scoping), so it must NOT enter the R010 duplicate-binding bookkeeping. + // Otherwise a real ROS app that legitimately binds the same node name later + // trips a phantom R010 hard error over a binding that is never used. + const std::string yaml = R"( +manifest_version: "1.0" +apps: + - id: "plc_process" + external: true + ros_binding: + node_name: "shared_node" + namespace: "/plant" + - id: "real_ros_app" + ros_binding: + node_name: "shared_node" + namespace: "/plant" +)"; + + auto manifest = parser_.parse_string(yaml); + auto result = validator_.validate(manifest); + + // R013 warns about the external app, but there is NO R010 error: the external + // app's binding is inert, so the real app's binding is unique among linkable + // apps. + EXPECT_TRUE(result.is_valid); + bool has_r010 = false; + for (const auto & e : result.errors) { + if (e.rule_id == "R010") { + has_r010 = true; + } + } + EXPECT_FALSE(has_r010) << "external app's inert binding must not trigger a phantom R010"; +} + // ============================================================================= // R011: Circular Dependency Detection // ============================================================================= diff --git a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp index bf30b175a..b4dafe9f2 100644 --- a/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp +++ b/src/ros2_medkit_gateway/test/test_merge_pipeline.cpp @@ -998,14 +998,15 @@ TEST_F(MergePipelineTest, ThreeLayerMerge_PerFieldGroupOwnerTracking) { // A protocol plugin introspects an external asset (e.g. a PLC over OPC UA) and // emits its App with external=true and no ROS binding. In hybrid mode the // manifest may declare the same App id as a bare stub (so a Function's -// hosted_by can reference it) without an `external:` key, which parses to the -// bool default false. `external` has no unset state, so the manifest's -// AUTHORITATIVE METADATA must not let that default erase the plugin's -// classification - collect_app_fqn() grants an empty-fqn App its bare-id fault -// scope only when external is true, so dropping the flag silently empties the -// App's fault rollup on every route that aggregates it. +// hosted_by can reference it) without an `external:` key, which leaves the +// tri-state classification unset (nullopt). The manifest's AUTHORITATIVE +// METADATA is the base, but a layer that does not classify must not shadow the +// plugin's classification - merge_external gap-fills it. collect_app_fqn() +// grants an empty-fqn App its bare-id fault scope only when external is true, +// so dropping the flag would silently empty the App's fault rollup on every +// route that aggregates it (#517). TEST_F(MergePipelineTest, PluginExternalClassificationSurvivesManifestMetadataMerge) { - // Manifest stub: declares the App id, no external key -> default false. + // Manifest stub: declares the App id, no external key -> stays unset (nullopt). App manifest_app = make_app("plc_process", "s7_1500"); manifest_app.source = "manifest"; @@ -1029,7 +1030,7 @@ TEST_F(MergePipelineTest, PluginExternalClassificationSurvivesManifestMetadataMe auto result = pipeline_.execute(); ASSERT_EQ(result.apps.size(), 1u); - EXPECT_TRUE(result.apps[0].external) << "hybrid merge dropped the plugin's external classification"; + EXPECT_TRUE(result.apps[0].external.value_or(false)) << "hybrid merge dropped the plugin's external classification"; // The manifest stays the entity's structural owner. EXPECT_EQ(result.apps[0].source, "manifest"); } @@ -1455,16 +1456,16 @@ TEST_F(MergePipelineTest, RuntimeComponentInUncoveredNamespaceNotSuppressed) { EXPECT_TRUE(found_motor) << "Runtime component in uncovered namespace should not be suppressed"; } -TEST_F(MergePipelineTest, AppExternalField_HigherPrioritySourceDefaultFalseCannotClear) { - // SOURCE-winner branch of the external merge: the base entity comes from a - // plugin (ENRICHMENT) that classified the app external; a later layer wins - // METADATA with AUTHORITATIVE but carries only the bool default false. - // `external` is monotone (#517): a layer that does not know the - // classification cannot clear it, regardless of its merge priority. +TEST_F(MergePipelineTest, AppExternalField_AuthoritativeOmittedCannotClearEnrichmentTrue) { + // SOURCE-winner branch of merge_external: the base entity comes from a plugin + // (ENRICHMENT) that classified the app external; a later AUTHORITATIVE layer + // wins METADATA but OMITS the classification (nullopt). Omission is a no-op - + // a layer that does not classify cannot clear one another layer expressed, so + // the plugin's external=true survives (#517). App plugin_app = make_app("plc_process", "s7_1500"); plugin_app.external = true; - App manifest_app = make_app("plc_process", "s7_1500"); + App manifest_app = make_app("plc_process", "s7_1500"); // external omitted -> nullopt LayerOutput plugin_out, manifest_out; plugin_out.apps.push_back(plugin_app); @@ -1479,7 +1480,37 @@ TEST_F(MergePipelineTest, AppExternalField_HigherPrioritySourceDefaultFalseCanno auto result = pipeline_.execute(); ASSERT_EQ(result.apps.size(), 1u); - EXPECT_TRUE(result.apps[0].external); + EXPECT_TRUE(result.apps[0].external.value_or(false)); +} + +TEST_F(MergePipelineTest, AppExternalField_AuthoritativeExplicitFalseOverridesPluginTrue) { + // The durable-fix capability a plain bool could not express: an explicit + // AUTHORITATIVE manifest `external: false` corrects a wrong plugin `external: + // true`. Production layer order - manifest is the base (AUTHORITATIVE), the + // plugin enters as ENRICHMENT source; TARGET wins and keeps the explicit + // false. This is the branch omission must NOT reach (that would resurrect the + // OR behavior). + App manifest_app = make_app("plc_process", "s7_1500"); + manifest_app.external = false; // explicit, authoritative: this IS a ROS node + + App plugin_app = make_app("plc_process", "s7_1500"); + plugin_app.external = true; // plugin misclassified it + + LayerOutput manifest_out, plugin_out; + manifest_out.apps.push_back(manifest_app); + plugin_out.apps.push_back(plugin_app); + + pipeline_.add_layer(std::make_unique( + "manifest", manifest_out, + std::unordered_map{{FieldGroup::METADATA, MergePolicy::AUTHORITATIVE}})); + pipeline_.add_layer(std::make_unique( + "plugin", plugin_out, + std::unordered_map{{FieldGroup::METADATA, MergePolicy::ENRICHMENT}})); + + auto result = pipeline_.execute(); + ASSERT_EQ(result.apps.size(), 1u); + ASSERT_TRUE(result.apps[0].external.has_value()); + EXPECT_FALSE(result.apps[0].external.value()) << "authoritative external: false must override plugin true"; } // --- Area field group merge tests --- @@ -1672,17 +1703,16 @@ TEST_F(MergePipelineTest, RuntimeLayerProducesNoAreasOrComponents) { EXPECT_EQ(result.apps[0].id, "sensor_node"); } -// --- External classification is monotone across layers (#517) --- +// --- External classification gap-fills across same-priority layers (#517) --- -// BOTH-winner branch (ENRICHMENT vs ENRICHMENT): a later layer's external=true -// classification is adopted even though the first layer already carried the -// bool default false. This deliberately supersedes the earlier first-set-wins -// pin (#260c): with a plain bool, false is indistinguishable from "not -// declared", and first-set-wins let a stub entry erase a real classification, -// silently dropping the app from every fault rollup (#517). -TEST_F(MergePipelineTest, AppExternalField_EnrichmentClassificationIsSticky) { - App layer1_app = make_app("controller", "nav_comp"); - layer1_app.external = false; +// BOTH-winner branch (ENRICHMENT vs ENRICHMENT): the base layer does not +// classify (nullopt), so a later same-priority layer's external=true gap-fills +// the classification. Omission never blocks a later classification. (An +// explicit false in the base would instead be kept - ties resolve to the base - +// which is the AuthoritativeExplicitFalseOverridesPluginTrue case; a plain bool +// could not tell the two apart, which is what regressed as #517.) +TEST_F(MergePipelineTest, AppExternalField_EnrichmentClassificationGapFills) { + App layer1_app = make_app("controller", "nav_comp"); // external omitted -> nullopt App layer2_app = make_app("controller", "nav_comp"); layer2_app.external = true; @@ -1698,8 +1728,8 @@ TEST_F(MergePipelineTest, AppExternalField_EnrichmentClassificationIsSticky) { auto result = pipeline_.execute(); ASSERT_EQ(result.apps.size(), 1u); - // Once any layer classifies the app as external, the classification stays. - EXPECT_TRUE(result.apps[0].external); + // A later layer's classification fills the base layer's gap. + EXPECT_TRUE(result.apps[0].external.value_or(false)); } TEST_F(MergePipelineTest, SuppressDoesNotAffectEmptyNamespaceEntities) { diff --git a/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py b/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py new file mode 100644 index 000000000..8888cef5e --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py @@ -0,0 +1,162 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""End-to-end regression for #517: external-app faults across every rollup. + +An external app (``external: true`` - e.g. a PLC bridged into SOVD by a protocol +plugin) reports faults to the fault_manager under its own entity id, not a ROS +FQN. Fault-scope resolution must recognise that bare id so the app's faults +surface on the app route AND on every aggregate route that rolls it up +(component, function, area). #517 was the fault scope resolving to a live FQN +instead of the bare id, which silently emptied those rollups. + +The fixture app deliberately also carries a ``ros_binding`` (the #517 "neighbor" +case): ``effective_fqn()`` derives a live FQN, so a fault scope that used it +would drop the app's bare-id faults. This test therefore fails against the +pre-fix fault scope and passes only when the external classification wins. + +The merge-level preservation is unit-covered +(``MergePipelineTest.PluginExternalClassificationSurvivesManifestMetadataMerge``) +and the per-route scope resolution is unit-covered +(``ResolveEntitySourceFqnsTest.*OwnsItsFaults``). This test pins the full +HTTP-stack behaviour those two disjoint unit layers never exercise together: +report a fault under the bare id, then observe it on every rollup route. +""" + +import os +import unittest + +from ament_index_python.packages import get_package_share_directory +import launch_testing +import rclpy +from rclpy.node import Node +from ros2_medkit_msgs.msg import Fault +from ros2_medkit_msgs.srv import ReportFault + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + + +EXTERNAL_APP = 'plc-process' +HOST_COMPONENT = 's7-plc' +HOST_FUNCTION = 'level-control' +HOST_AREA = 'plc-cell' +FAULT_CODE = 'PLC_LEVEL_OVERFLOW' + + +def generate_test_description(): + manifest_path = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', 'examples', 'external_app_fault_manifest.yaml', + ) + return create_test_launch( + demo_nodes=[], + fault_manager=True, + # Negative threshold: the fault confirms after a couple of FAILED + # events (see _report_fault, which reports several times). + fault_manager_params={'confirmation_threshold': -2}, + gateway_params={ + 'discovery.mode': 'hybrid', + 'discovery.manifest_path': manifest_path, + 'discovery.manifest_strict_validation': False, + }, + ) + + +class TestExternalAppFaultRollup(GatewayTestCase): + """An external app owns its faults on the app route and every rollup.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {EXTERNAL_APP} + REQUIRED_AREAS = {HOST_AREA} + REQUIRED_FUNCTIONS = {HOST_FUNCTION} + + @classmethod + def setUpClass(cls): + # Wait for the gateway + discovery (the external app, area and function + # must exist before we exercise the rollup routes). + super().setUpClass() + rclpy.init() + cls._reporter = Node('external_app_fault_reporter') + cls._report_client = cls._reporter.create_client( + ReportFault, '/fault_manager/report_fault' + ) + assert cls._report_client.wait_for_service(timeout_sec=15.0), \ + 'report_fault service not available' + + @classmethod + def tearDownClass(cls): + cls._reporter.destroy_node() + rclpy.shutdown() + + def _report_fault(self, times=4): + """Report the external app's fault under its bare entity id. + + Reported several times so the negative ``confirmation_threshold`` + latches the fault to CONFIRMED regardless of debounce timing. + """ + for _ in range(times): + req = ReportFault.Request() + req.fault_code = FAULT_CODE + req.event_type = ReportFault.Request.EVENT_FAILED + req.severity = Fault.SEVERITY_ERROR + req.description = 'PLC tank level exceeded high limit' + req.source_id = EXTERNAL_APP + future = self._report_client.call_async(req) + rclpy.spin_until_future_complete( + self._reporter, future, timeout_sec=5.0 + ) + self.assertIsNotNone( + future.result(), 'report_fault call timed out' + ) + self.assertTrue( + future.result().accepted, 'fault_manager rejected the report' + ) + + def test_external_app_fault_appears_on_every_rollup(self): + """The external app's fault surfaces on app, component, function, area. + + @verifies REQ_INTEROP_012 + """ + self._report_fault() + + # Precondition: the owning app sees the fault. If this fails the setup + # is wrong (not the rollup), so assert it first for a clear signal. + self.wait_for_fault(f'/apps/{EXTERNAL_APP}', FAULT_CODE) + + # #517: the same fault must roll up to every aggregate route that + # includes the external app. Before the fix these rollups were empty + # because the app's bare-id fault scope was dropped. + for endpoint in ( + f'/components/{HOST_COMPONENT}', + f'/functions/{HOST_FUNCTION}', + f'/areas/{HOST_AREA}', + ): + with self.subTest(rollup=endpoint): + fault = self.wait_for_fault(endpoint, FAULT_CODE) + self.assertEqual(fault.get('fault_code'), FAULT_CODE) + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + + def test_exit_codes(self, proc_info): + """Check all processes exited cleanly (SIGTERM allowed).""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}' + )