fix(gateway): preserve plugin App external flag through hybrid merge

docs/config/discovery-options.rst

@@ -175,6 +175,19 @@ 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`` 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
 (``authoritative``, ``enrichment``, ``fallback``):

docs/config/manifest-schema.rst

@@ -550,7 +550,16 @@ Fields
    * - ``external``
      - boolean
      - No
-     - True if not a ROS node (default: false)
+     - 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 and
+       fault scoping - the app is scoped by its entity id).
 
 ros_binding Fields
 ~~~~~~~~~~~~~~~~~~

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

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<std::string> 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<bool> 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

src/ros2_medkit_gateway/src/core/discovery/manifest/manifest_validator.cpp

@@ -186,6 +186,21 @@ 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 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 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;
 
       // For topic namespace bindings

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

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));

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<bool>() : 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<bool>();
+  }
   app.source = "manifest";
 
   // Parse ros_binding

src/ros2_medkit_gateway/src/discovery/manifest/runtime_linker.cpp

@@ -83,7 +83,7 @@ LinkingResult RuntimeLinker::link(const std::vector<App> & 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;
     }

src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp

@@ -169,6 +169,30 @@ void merge_optional(std::optional<T> & target, const std::optional<T> & 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<bool> & target, const std::optional<bool> & 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,11 +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);
-        // 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)
+        merge_external(target.external, source.external, res.scalar);
         break;
     }
   } else if constexpr (std::is_same_v<Entity, Function>) {

src/ros2_medkit_gateway/test/test_handler_context.cpp

@@ -1699,6 +1699,65 @@ TEST(ResolveEntitySourceFqnsTest, ComponentHostingExternalAppOwnsItsFaults) {
   EXPECT_EQ(fqns, std::set<std::string>{"process"});
 }
 
+TEST(ResolveEntitySourceFqnsTest, FunctionHostingExternalAppOwnsItsFaults) {
+  // GET /functions/<id>/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<std::string> expected{"process", "/perception/nav/planner"};
+  EXPECT_EQ(fqns, expected);
+}
+
+TEST(ResolveEntitySourceFqnsTest, AreaHostingExternalAppOwnsItsFaults) {
+  // GET /areas/<area>/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<std::string>{"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<std::string>{"process"});
+}
+
 int main(int argc, char ** argv) {
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();

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) {