fix(gateway): preserve plugin App external flag through hybrid merge#522
fix(gateway): preserve plugin App external flag through hybrid merge#522bburda wants to merge 2 commits into
Conversation
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
There was a problem hiding this comment.
Pull request overview
Fixes a hybrid-discovery merge bug in ros2_medkit_gateway where a protocol plugin’s App.external=true classification could be lost when the manifest declared the same App ID without an external: key, causing external-app faults to disappear from aggregated fault rollups (notably Functions).
Changes:
- Make the
App.externalmerge monotone in the discovery merge pipeline (once external, always external). - Update and add unit tests to pin the new merge behavior and cover Function fault-scope rollups for external apps.
- Document the monotone-merge exception for
externalin discovery options and the manifest schema.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ros2_medkit_gateway/test/test_merge_pipeline.cpp |
Adds/updates unit tests to reproduce #517 and pin monotone external merge semantics. |
src/ros2_medkit_gateway/test/test_handler_context.cpp |
Adds regression coverage for Function fault-scope rollup when hosting an external app directly. |
src/ros2_medkit_gateway/src/discovery/merge_pipeline.cpp |
Changes external merge to monotone (OR across layers) to preserve plugin classification through hybrid merge. |
docs/config/manifest-schema.rst |
Documents that omitting external in hybrid mode won’t clear an external classification from another layer. |
docs/config/discovery-options.rst |
Adds a note calling out external as an exception that merges monotonically in hybrid mode. |
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.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| } | ||
| // 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 |
There was a problem hiding this comment.
The durable fix is optional<bool>. This is the second flip of the same coin - 4e01daf removed the OR ("classification, not a status flag"), this restores it ("no layer can un-classify") - both right about different halves of a distinction a plain bool can't encode (manifest_parser:381 collapses explicit false and omission). Side effect of the OR: an authoritative manifest external: false can never correct a wrong plugin classification. Worth a follow-up issue.
| result.add_warning("R013", | ||
| "App '" + app.id + | ||
| "' declares external: true together with a ros_binding; " | ||
| "the binding is ignored for linking", |
There was a problem hiding this comment.
"ignored for linking" is true but understates it - effective_fqn() (app.hpp:102) still derives an FQN from the binding, so the app's fault scope points at a dead ROS FQN instead of its bare id (the exact bug this PR fixes). Suggest "...ignored for linking and the app loses bare-id fault ownership". Same wording in manifest-schema.rst:557.
| "' declares external: true together with a ros_binding; " | ||
| "the binding is ignored for linking", | ||
| "apps/" + app.id + "/ros_binding"); | ||
| } |
There was a problem hiding this comment.
After the R013 warning it still inserts this external app's binding_key into seen_bindings (line 212), so a real ROS app reusing that node name later fails R010 (hard error) over a phantom conflict. A continue here (or skipping the bookkeeping for external apps) fixes it.
| - 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 |
There was a problem hiding this comment.
"omitting it does not clear" reads like an explicit external: false would win - it doesn't (parser collapses it, the OR discards it, no log). Suggest: "neither omitting external: nor setting it to false clears a classification contributed by another layer".
| 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 |
There was a problem hiding this comment.
Same as the manifest-schema note - with OR semantics no layer clears the flag, including an explicit external: false. Reword to "no layer can clear it - neither omitting external: nor setting it to false has any effect once another layer classified the app external".
| EXPECT_EQ(fqns, std::set<std::string>{"process"}); | ||
| } | ||
|
|
||
| TEST(ResolveEntitySourceFqnsTest, FunctionHostingExternalAppOwnsItsFaults) { |
There was a problem hiding this comment.
AREA is the only rollup without an external-app test now (App/Component/Function have one). The AREA route walks the same gate (fault_scope.cpp:121 -> collect_area_app_fqns -> collect_app_fqn) and is production-real - the opcua plugin sets comp.area, so GET /areas//faults depends on the same branch. An AreaHostingExternalAppOwnsItsFaults sibling closes the set.
| // 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) { |
There was a problem hiding this comment.
Worth an end-to-end pin for #517: external appears nowhere in the integration tests, and the full chain (hybrid stub + plugin app -> merge -> fault under bare id -> visible in the rollup) only exists as two disjoint unit layers. This exact interaction regressed once via a semantics-only refactor (4e01daf) that every unit test of its era survived - an integration test is the layer that catches it.
Pull Request
Summary
Under
discovery.mode: hybrid, the merge pipeline dropped theexternal=trueclassification that a protocol plugin set on an App when the manifest declared the same app id as a bare entry (noexternal:key). The manifest layer wins the METADATA field group, and a plain bool cannot distinguish "not declared" from "declared false", so the manifest default erased the plugin classification. Without the flag the app lost its bare-id fault scope, and faults it reported vanished from Function and Component fault rollups.manifest_onlywas not affected because it bypasses the merge pipeline.The fix makes the
externalmerge monotone: once any discovery layer classifies an app as external, a layer that does not carry the classification cannot clear it. This replaces the earlier scalar merge semantics for this field, and the two unit tests that pinned them are updated to the new behavior. Docs describe the exception in the discovery options and manifest schema pages.Also adds the missing fault-scope regression for a Function that hosts an external app directly (the App and Component routes already had one).
Issue
Type
Testing
MergePipelineTest.PluginExternalClassificationSurvivesManifestMetadataMergereproduces the report: manifest stub + plugin app withexternal=true, merged app must stay external. It fails without the fix and passes with it.AppExternalField_HigherPrioritySourceDefaultFalseCannotClearandAppExternalField_EnrichmentClassificationIsStickycover the other two merge-winner branches.ResolveEntitySourceFqnsTest.FunctionHostingExternalAppOwnsItsFaultscovers the Function fault-scope rollup.Checklist