50 Commits

Author	SHA1	Message	Date
romanett	414957c88a	Reduce ISubscription: internalize the publish pipeline, delete the dead members (#4349) ... # Description Reduces the server subscription API surface per the measurements from issue #4185. ## ISubscription: 42 → 30 members (#4185) A call-site census of the 11 "publish-pipeline protocol" members plus `Publish` (all 68 `src/` projects, samples, tools, fuzzing, tests) found **zero callers outside `Opc.Ua.Server`** and split the members three ways: - **Deleted (4):** `ItemReadyToPublish` and `ItemNotificationsAvailable` were dead — zero callers repo-wide and implementation bodies commented out since before 1.5.378 — the obsolete parameterless `SessionClosed()` had zero callers, and `TransferSessionAsync` (the one-shot direct transfer, which bypassed the reservation protecting a transfer against a concurrently closing source session) lost its last caller once the manager always uses the claim/prepare/commit protocol. - **Internalized (8):** `PublishTimerExpired`, `Acknowledge`, `PublishTimeout`, `SubscriptionTransferred`, `AvailableSequenceNumbersForRetransmission`, `QueueOverflowHandler`, `SessionClosed(ISession)` and `Publish` move to `internal interface ISubscriptionPublishPipeline : ISubscription`, implemented **explicitly** by `Subscription` (the `ISubscriptionMonitoredItemLifecycle` shape) so they are unreachable through the public concrete class too. `Publish` goes with them deliberately — it is the same hazard class (consumes notifications, advances sequence numbers) with the same single production caller. - **Kept public (1):** `ResendData` drops out of the issue's list — its only caller is the `Server_ResendData` UA-method handler in `DiagnosticsNodeManager`, resolving via `TryGetSubscription` like `GetMonitoredItems` does. It is service-facing, not pipeline. ## The pipeline contract is complete: SessionPublishQueue traffics only in it `SessionPublishQueue` becomes `internal sealed` (zero consumers outside the assembly; `SentMessageQueue` already was) and works **solely with `ISubscriptionPublishPipeline`** — no `ISubscription` and no concrete `Subscription` anywhere in the file. To get there, the internal transfer-claim protocol (`TryBeginTransfer`, `PrepareSessionTransferAsync`, `CompleteTransfer`, `AbortTransfer`, `TryRestoreSessionAfterFailedTransfer`) moved from `internal` methods on the concrete class onto the internal interface, implemented explicitly. That completes the contract, so `TransferSubscriptionsAsync` loses its two-tier shape: the ~6 `subscription as Subscription` null-guard branches and the degraded `TransferSessionAsync` fallback for non-`Subscription` implementations are gone — one code path, always the claim/prepare/commit/rollback protocol, and `ISubscription.TransferSessionAsync` is deleted with it. Subscription creation fails with `Bad_InternalError` when a `CreateSubscription` override returns a type not derived from `Subscription`, instead of publishing partially — such implementations were already second-class (transfer required the concrete type before this change). This is the headline behavior change for exotic extenders and is documented prominently in `docs/migrate/2.0.x/sessions-subscriptions.md`. The durable-subscription open question in the issue is answered **no**: `Subscription/Persistence/` never references `ISubscription` (the store deals in `IStoredSubscription` DTOs; restore constructs the concrete type via `Subscription.RestoreAsync` inside the manager), so the internalization cannot break a store implementer. ## #4186: measured, prototyped, and deliberately kept as-is Body inspection showed only 4 of the 10 `ISubscriptionManager` wrappers are pure lookup-and-delegate (`Republish`, `SetTriggering`, `ModifyMonitoredItemsAsync`, `SetMonitoringModeAsync` — one caller each); the other 6 carry logic with no `ISubscription` substitute. A collapse of the four was implemented and then reverted on this branch: it traded ten uniform id-addressed members for a mixed dispatch shape in `StandardServer` without reducing what a reader must understand. **Every subscription service keeps going through `ISubscriptionManager`**; the wrapper bodies now resolve against the pipeline-typed registry. The decision is recorded in `plans/ServerRuntimeAnalysis.md` (open question 5). ## Compatibility treatment Removals are outright, per the documented no-`[Obsolete]`-shim policy for these interfaces (`docs/MigrationGuide.md` — re-adding an interface member would break every implementer; net48/netstandard2.0 have no default interface methods). In its place: - Migration analyzer **UA0030** (pipeline members + `SessionPublishQueue`, distinguishing deleted no-ops from internalized protocol), with tests, following the UA0024 syntax-based pattern. The transfer-claim members were never public in 1.5.378, so they need no migration story. - `docs/MigrationGuide.md#ua0030` section with before/after code; the `docs/migrate/2.0.x/sessions-subscriptions.md` section that told implementers to *add* `SessionClosed(ISession)` is replaced with the derive-from-`Subscription` story; the 2.0.x README symptom table routes the rule. - All 11 removed/internalized members existed in 1.5.378 (verified against `master378`), so each is documented as a 2.0 break. ## Tests Converted, not deleted. `Mock<ISubscription>` setups of pipeline members become `Mock<ISubscriptionPublishPipeline>` — Moq proxies the internal interface via a new `InternalsVisibleTo("DynamicProxyGenAssembly2")` (keyless; the SDK appends `$(PublicKey)` under `SignAssembly`, and signed pipelines do not run tests, matching the existing `Opc.Ua.Redundancy.Kubernetes` precedent). A test-side extension shim forwards the original member names (pipeline + transfer protocol) so the direct call sites on concrete `Subscription` instances compile unchanged. The transfer-race tests' typed reflection probes into `m_abandonedSubscriptions` follow the field's new type. - `Opc.Ua.Server.Tests` (net10.0): 4795 tests, 0 failures - `Opc.Ua.Subscriptions.Tests`, `Opc.Ua.Subscriptions.Durable.Tests`, `Opc.Ua.Subscriptions.Classic.Tests` (net10.0): green - `Opc.Ua.MigrationAnalyzer.Tests`: green incl. the new UA0030 tests - net48 gate: solution build green; subscription-area fixtures green (the known pre-existing local hosting-test environment failures reproduce identically on unmodified master) ## Related Issues - Fixes #4185 - #4186 was measured and prototyped on this branch and deliberately **not** taken (see the section above); the decision is recorded in `plans/ServerRuntimeAnalysis.md`. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. _(net10 suites fully green locally; net48 build green and subscription-area fixtures green — the remaining net48 hosting-test failures on my machine reproduce identically on unmodified master.)_ - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-31 10:53:51 +00:00
romanett	14fba2d120	Fix instance TypeDefinition across dependency ModelDesign boundaries (#4355) ... # Description The model source generator crashed with `MODELGEN003` / `NullReferenceException` whenever a `ModelDesign` instantiated an `ObjectType` (or `VariableType`) declared in a **dependency** design file, as soon as that type had children. The sibling of #4332 on the instance-declaration path: #4335 linked dependency *data types*, but an *instance* whose `TypeDefinition` lives in another design still crashed in `NodeStateGenerator` on the type's inherited children. Root cause: dependency design files load with `validateDictionary: false`, so `ValidateInstance` / `ValidateType` never ran on them and their types' children kept `TypeDefinitionNode` / `DataTypeNode` at `null`. When the target instance merged those children into its hierarchy (`BuildInstanceHierarchy` re-walks the dependency type's `Children.Items`), the null references were copied along, and `GetNodeStateClassName` dereferenced them while building the node state factory list. Changes: - A new best-effort `LinkDependencyInstances` pass in `ModelDesignValidator` links dependency-design instances the way `ValidateInstance` / `ValidateType` do for the target model: the `TypeDefinitionNode` / `DataTypeNode` of the types' children and standalone instances (recursively), variable `DataType` / `ValueRank` inheritance from the variable type, `DefaultValue` decoding, `VariableTypeDesign.DataTypeNode` restrictions, and method arguments — including creation of the `InputArguments` / `OutputArguments` argument properties so method children materialise into the target hierarchy exactly like local ones. Resolution is best effort (unresolvable references stay `null`) and NodeSet2 dependencies remain excluded, consistent with `LinkDependencyDataTypes`. - The pass runs alongside `LinkDependencyDataTypes`: for upstream dependencies after all of them are loaded and before the target validates (so the hierarchy merge picks up the links), and for downstream dependencies after they load. - `docs/ModelDependencies.md` documents the instance linking. Tests (all verified to fail with the exact reported NRE stack before the fix): - `CrossModelInstanceTypeDefinitionTests` (Core, 4 tests): the issue's minimal repro end to end; an equivalence test asserting the cross-model instance emits the **identical node-state factory set** as a locally declared type (this is the issue's stated expectation, and it drove the method-argument-property part of the fix); the flipped upstream/downstream ordering; and validator-level assertions that the merged hierarchy carries linked `TypeDefinitionNode` / `DataTypeNode` references, including the data type inherited from a dependency `VariableType`. - `CrossModelInstanceTypeDefinitionGeneratorTests` (Roslyn driver, 2 tests): the issue's csproj repro shape — both `AdditionalFiles` orderings, asserting no diagnostics and that the combined generated output of both models **compiles** (the instance factories reference the dependency model's typed state classes). Verified locally on net10.0: `Opc.Ua.SourceGeneration.Core.Tests` 3800 passed, `Opc.Ua.SourceGeneration.Tests` 141 passed, `Opc.Ua.SourceGeneration.Stack.Tests` 94 passed; 0-warning Release build of `Opc.Ua.SourceGeneration.Core` (all TFMs incl. netstandard2.0); `dotnet format whitespace --verify-no-changes` clean. This removes the last generator blocker for source generating the instance model of the `Workshop/DataTypes` sample (OPCFoundation/UA-.NETStandard-Samples#814): its `ParkingLot` / `DriverOfTheMonth` are instances of types declared in the other design file. ## Related Issues - Fixes #4353 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>	2026-08-31 09:47:08 +00:00
romanett	24ba4855e6	Fix [DataType] generator Clone() dropping every inherited field of a derived structure (#4354) ... # Description The `[DataType]` source generator emitted a `Clone()` that created a fresh instance and copied only the fields declared on the annotated type itself, so every field inherited from an `IEncodeable` base was silently dropped. Since the server clones node values while loading an address space, a derived structure served by a node manager came back out of a Read with its inherited fields empty, even though encode/decode was correct. This PR makes the `[DataType]` path emit the same `MemberwiseClone`-based shape as the model-based generator: ```csharp /// <inheritdoc/> public override object Clone() { return (DerivedStruct)this.MemberwiseClone(); } /// <inheritdoc/> public new object MemberwiseClone() { DerivedStruct clone = (DerivedStruct)base.MemberwiseClone(); // deep copy of the own reference-typed fields return clone; } ``` `Clone()` delegates to a generated `public new object MemberwiseClone()` that starts from `base.MemberwiseClone()` — a shallow copy of the whole object, inherited fields included — and then deep copies the type's own reference-typed fields (encodeables, `DataValue`, `Variant`, `ExtensionObject`). Derived classes hide `MemberwiseClone` and chain into the base implementation, so deep-copy semantics compose across the hierarchy, including across assemblies into model-generated base types (the `BicycleType` scenario that surfaced this). The two generator paths now agree. Because the clone now starts from a full shallow copy (`base.MemberwiseClone()` for classes, `with { }` for records), the per-field clone emitter only writes the deep-copy assignments; the redundant per-field plain copies are gone. This also fixes a latent side bug in the class path: init-only simple fields were silently dropped by the fresh-instance clone as well. Records were already correct (`with { }` carries every field) and are unchanged in shape. Tests: - `GenerateClassCloneUsesMemberwiseClone` / `GenerateDerivedClassCloneCarriesInheritedState` assert the emitted shape for root and derived classes. - `DerivedClassCloneKeepsInheritedFields` runs the exact repro from the issue through the Roslyn driver, emits the assembly, and executes `Clone()` at runtime to prove the inherited field survives (verified on net10.0 and net48). Verified locally: `Opc.Ua.SourceGeneration.Core.Tests` (3798 passed), `Opc.Ua.SourceGeneration.Tests` (140 passed), `Opc.Ua.SourceGeneration.Stack.Tests` (94 passed), and `Opc.Ua.Client.Tests` (2161 passed, exercises the regenerated in-tree `[DataType]` record types). The in-tree `[DataType]` consumers (Opc.Ua.Client, both GDS common projects) regenerate and build without warnings. ## Related Issues - Fixes #4352 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-30 22:10:27 +00:00
romanett	766c5558e8	Fix structure subtyping across dependency ModelDesign boundaries (#4335) ... # Description The model source generator crashed whenever a `ModelDesign` declared a structure that subtypes a structure from **another** design file (`MODELGEN003` with a `NullReferenceException`), and the cross-assembly `[assembly: ModelDependency]` flow failed even earlier with `The BaseType reference ... is not the expected type: DataTypeDesign`. Structure inheritance across model boundaries — the primary use case of companion-spec layering — could not be source-generated at all. Three root causes, all in the dependency-loading path of the validator: 1. **Dependency design files were never linked.** They load with `validateDictionary: false`, so their `DataTypeDesign`s kept `BasicDataType` at the enum default (`Boolean`) and their fields' `DataTypeNode` stayed `null`. `BinarySchemaGenerator`'s inherited-field merge then dereferenced the null `DataTypeNode`, and `XmlSchemaGenerator` / `DataTypeGenerator` silently emitted `xs:boolean` / `bool` from the default classification. 2. **Dependencies that import the target crashed at load.** Each generation group receives every design file of the compilation as a dependency, including downstream models that import the group's target — those loaded *before* the target they import and failed resolving their `BaseType`. This produced the confusing `AdditionalFiles`-order-dependent failure modes. 3. **The payload flow never reached ModelDesign targets.** `ModelCompilation` supplied `referencedDependencies` only to the NodeSet2 pass, never to the design-file pass, and payload-materialised data types lacked the classification and field links needed to serve as base types of local structures. Changes: - `ModelDesignValidator.ValidateModel` loads dependencies in two phases around the target: upstream models first (topological, referenced models before their referrers), then the target, then models that import the target. - A new `LinkDependencyDataTypes` pass classifies dependency data types (`IsStructure` / `IsEnumeration` / `BasicDataType`) and resolves field `DataTypeNode` / `Parent` best effort. It runs for payload-imported declarations and upstream design-file dependencies before target validation, and for downstream dependencies after they load. NodeSet2 dependencies are excluded — `NodeSetToModelDesign` already links and classifies them with its own semantics. - Payloads for models that are backed by a design file in the compilation are skipped — the explicit file is authoritative (previously, importing both crashed with duplicate symbolic ids). - Payload-materialised data types honor the payload's `IsEnumeration` flag. - The design-file `GenerateCode` pass now receives the referenced assemblies' dependency payloads (`ModelCompilation` + new optional `referencedDependencies` parameter). - `docs/ModelDependencies.md` updated accordingly. Tests (all verified to fail with the exact reported errors before the fix): - `CrossModelStructureSubtypeTests` (Core, 6 tests): the issue's minimal repro end to end — asserting the emitted `.bsd` carries inherited fields with correct types and `SourceType`, the `.xsd` extends the dependency structure (not `xs:boolean`), and the generated C# derives correctly — plus the reversed target/dependency order, both payload flows at generation and validator level, and a three-level chain (target → payload structure → design-file structure). - `CrossModelStructureSubtypeGeneratorTests` (Roslyn driver, 3 tests): both `AdditionalFiles` orderings compiling the generated output, and the full cross-assembly flow (producer assembly generated from the upstream design; consumer resolves the base structure from the reference's payload without re-emitting it). Verified locally on net10.0: `Opc.Ua.SourceGeneration.Core.Tests` 3786 passed, `Opc.Ua.SourceGeneration.Tests` 135 passed, `Opc.Ua.SourceGeneration.Stack.Tests` 94 passed; 0-warning Release builds of `Opc.Ua.SourceGeneration.Core` (all TFMs incl. netstandard2.0), both Roslyn generator variants, Stack and Tester. This unblocks the `[NodeManager]` migration of the DataTypes workshop sample (OPCFoundation/UA-.NETStandard-Samples#757). ## Related Issues - Fixes #4332 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>	2026-08-30 17:22:34 +00:00
Marc Schier	cef83550cb	OPC UA Vision: a vision-guided bin-picking demo an LLM agent can see and drive (#4235) ... # Description Implements **OPC UA — Vision** and uses it to build the demo the companion specifications were drafted for: an eye-in-hand camera watches a bin of parts, a language model *looks at the frame*, decides which part matches the instruction, and commands the robot through Robot Intent — with the OpenUSD viewer showing the arm move and the bin empty out. > **Draft, and stacked.** [#4195](https://github.com/OPCFoundation/UA-.NETStandard/pull/4195) has merged and master is merged in, so what is left on top of `master` is the Vision work plus the unmerged `marcschier/ai-model-management` branch, which the demo needs for the model/dataset/deployment provenance. Review the Vision commits; the AI Model Management work lands with its own PR. ## What is here **The specification, implemented.** `Opc.Ua.Vision` is generated from the spec's own `Opc.Ua.Vision.NodeSet2.xml`, vendored byte-identically (SHA-256 `79C592C6…`, verified against a fresh download and stored LF-preserved like the Robot Intent NodeSet). `Opc.Ua.Vision.Server` builds the §4.2 discovery object, sensors, the coordinate-frame tree, calibrations, media endpoints, inference pipelines and the §9 feedback methods, with rendering and inference behind injectable providers. `Opc.Ua.Vision.Client` is built on the generated ObjectType proxies and composes transforms across the frame tree, which is what turns a detection in camera coordinates into a pose a robot can act on. **Two perception paths behind one contract**, which is exactly what `InferenceLocationEnum` is for. `OnServer` is a deterministic detector derived from the stage's ground truth — no model, no network, no GPU, so CI can exercise the whole loop. `EdgeOffServer` is the agent: it sees the frame over MCP and calls `SubmitDetections`, and the Server publishes results it did not compute. A client reads `DetectionResultType` identically either way. **The agent's eyes.** `tools/Opc.Ua.Mcp.Vision` adds 22 `vision_*` tools, which with the four shared connection tools makes the `vision` profile 26; `vision_get_frame` returns the camera image as an MCP `ImageContentBlock`, so the model looks at pixels rather than reading a description of them. Tool profiles now compose — `--profile vision,robotics` yields 62 tools — so one agent can both see and act without dragging in the full 136-tool catalogue. **The cell.** `samples/Robotics/BinPickingCell` follows the spec's own Robotics-Vision Addendum: the `world → robot_base → flange → gripper_tcp` frame tree with `camera_eih` on the flange, the `HandEye` extrinsics and the `Intrinsics612x512` intrinsics — the addendum's 2448×2048 calibration scaled to the 4×4-binned grid the camera actually delivers. `samples/Robotics/BinPickingClient` hosts the composed MCP catalogue and optionally the viewer. **Documentation.** `docs/Vision.md` (~800 lines) covers the model, hosting, the build context, every topology builder, both perception paths, feedback, the MCP profile and the limitations; linked from `docs/README.md`, with the profile table in `docs/McpServer.md` and READMEs for both samples. ## Evidence, not assertions Each of these was measured on a running system rather than assumed: | Claim | Evidence | |---|---| | The camera sees the bin | 612×512 frame with the bin and all five parts in view — red `(212,47,47)`, green `(61,193,71)`, blue `(54,85,218)`, yellow `(215,196,58)`, orange `(209,119,55)` | | An agent can act on what it sees | Over MCP, `vision_get_frame` returns a 612×512 PNG that decodes from base64 to 1,254,051 bytes, the descriptor beside the detections says 612×512, and all five `BoundingBox2D` centres land **inside** the frame on the pixel colour their own detection claims | | The geometry is right | A detection composed `camera_eih → world` lands on the authored position, residual **0.0000 m** — on both the on-server and off-server paths | | The detector tracks the world | Placing a part drops the next scan from 5 detections to 4, with that part absent | | The robot obeys the agent | Scripted run took command authority and completed a pick and a place, both `Succeeded` | | Tool catalogues are honest | Counted from a running server: vision 26, robotics 40, `vision,robotics` 62, full 136, **zero duplicates** | | Every node is browsable by NodeId | Eight representative NodeIds — both folders, a pipeline, a frame, the sensor, its Calibrations folder, the HandEye calibration, the Media folder — all `Good` | That tool-count check matters because the profile tests collapse names into a `HashSet` and *cannot see* a duplicate registration — so the new tests count registrations, not names. ## Things that were wrong, and are not now Writing the tests found three defects of one kind — code that answered instead of refusing. A zero-norm quaternion was silently rewritten to identity, so a caller with an all-zero orientation got a confident wrong grasp pose. `SubmitCorrection` rewrote a null `ResultId` to empty and called the sink anyway. A bad `StageIdentifier` **killed the host process** inside native `UsdStage.Open`, via an exception .NET cannot catch. All three now refuse loudly and are pinned by tests. **Nodes that existed but could not be reached.** The node manager indexes the Vision root before configurators run, so everything the fluent builder grafted on afterwards — both folders, the pipelines with their Results and Feedback, the frames, the sensors with their optics, calibrations and media endpoints — was never added to the index. Browsing *forward from a parent* worked, because that walks `NodeState.Children` in memory; browsing any of those nodes *by its own NodeId* returned `BadNodeIdUnknown`, which is how an ordinary client and the MCP discovery tools navigate. Registration is now deferred and flushed after each configurator. The first fix covered only the DI path and left the documented non-DI fallback broken in exactly the same way, so `ConfigureVisionAsync` now exists and registers what it builds. That defect survived a green suite because every test browsed children. The new tests assert by NodeId, and were checked against the broken code — with the flush removed, they fail. **Every Method was uncallable.** The Server passed 426 unit tests while not one of its Methods could be invoked over a real session — `RunInference` returned `Bad_TooManyArguments`, and so did every other Method that takes arguments, which is all of them but `StartContinuous` and `Stop`. Three independent faults, each sufficient on its own: the Method nodes were never created (they are Optional children, and the dispatcher guards every attachment with a null check, so it silently attached nothing); nothing declared the arguments, so the stack concluded none were expected; and `MethodDeclarationId` pointed at a synthesised NodeId rather than the NodeSet declaration a client calls with, so the lookup missed the instance Method entirely. The builder now creates the Methods, the `Results` folder and the `Feedback` object when a provider or sink is configured, declares every signature, and names the right declaration. Unit tests could not have caught this: they invoke the handler delegates directly and never go through `MethodState.Call`. It took an end-to-end test over a real session. Relatedly, children created by the generated `CreateOrReplace` helpers carry no `ReferenceTypeId`, so nothing could reach them — fixed once in the build context rather than at the several dozen call sites. **And one where I was wrong.** Earlier in this branch I reversed my own agents' decision and made `SubmitDetections` accept an empty detection set, reasoning that "I looked and the bin is empty" is a correct observation and refusing it forces a correct agent to invent a detection. The reasoning is sound; it is also an argument *against* the specification rather than an implementation of it. §9.5 is normative and explicit — its StatusCode table lists `Bad_InvalidArgument` for "`Detections` empty; or `SubmitCorrection` supplies both or neither corrected array". Worse, **none of §9.5's argument rules were enforced anywhere** — the dispatcher forwarded every submission straight to the feedback sink, so conformance depended on whichever sink a host installed. They are Server obligations, so they now live in the dispatcher, ahead of the sink, proven by strict mocks that would throw if the sink were consulted. The usability problem is real and does not go away by conforming, so it is raised where it can be fixed: [opcua-drafts#70](https://github.com/marcschier/opcua-drafts/issues/70). ## Tracking the updated drafts The Vision, Robot Intent and AI Model Management NodeSets on `opcua-drafts` `main` have moved, partly because of feedback this branch raised. All three are re-vendored byte-identically — verified by comparing git blob hashes against the upstream files rather than by eye — and the implementations follow. **Two statements that were previously inexpressible now have a wire representation.** `SubmitDetections` takes `SceneIsEmpty` and `SubmitCorrection` takes `RetractAll`, so an agent can report "I examined this frame and there is nothing in it" — the terminating condition of the bin-picking task — and can retract a false positive by correcting a result down to nothing. Both are checked in both directions, because the flag is exactly what separates a deliberate empty observation from a lost payload: an empty array without the flag is refused, and the flag with an array attached is refused as two contradictory claims about one frame. Implemented in the dispatcher, the client, the MCP tools the agent drives, and the sample cell. Also absorbed: `LampType` and `LightingMode` became enums (§66), `ProfileName` became `DefaultProfileName` on the endpoint (distinct from the `ProfileName` a caller passes to `GetStreamEndpoint`, which keeps its name), AI `Invoke`/`InvokeAsync` gained `PayloadUri` under the same exactly-one rule, and AI `ListModels` gained a `ContinuationPoint` — because `MaxResults` alone puts everything past the bound permanently out of reach. **OpenUSD is on `0.9.0-alpha`**, verified on the running cell rather than by restore alone: D3D12 backend, 612×512 frame, the bin and all five parts in view and detected — the same result `0.8.0-alpha` produced. The one local workaround that survives is `VisionMethodArguments`, and its reason has changed rather than gone away: the builder materialises these Methods as Optional children through `CreateOrReplace`, which constructs the state object directly and never runs the generated factory that now carries the signatures. Checked rather than assumed — removing the declarations fails five of the eight `VisionMethodSurfaceTests`. ## Review feedback Three threads, all addressed in `0fe2803a3`: - **The AI Model Management work is now a library family, not a sample.** `src/Opc.Ua.AI` (model), `.Inference` (`IInferenceBackend` and backends), `.Server` (node manager) and `.Client` (discovery, reads, calls, artefact transfer), in the same shape as the Robotics and Vision families; the samples become `samples/AI/ModelManagementServer` and `ModelManagementClient`. The client library needed real work rather than relocation: its browsing surface was `private` inside the sample's scenario runner, so moving the file alone would have produced a library nobody could call. - **No cloud-vendor SDK is referenced anywhere.** Inference goes through `Microsoft.Extensions.AI` — an `IChatClient` is implemented by hosted services and on-device runtimes alike, which is exactly the property clause 8.1 asserts. `Azure.Identity` is gone: workload identity is not an Azure feature, every platform projects a token into a file and each SDK reads that file behind its own API, so the resolver reads it directly. The comment claiming `Microsoft.Extensions.AI` would force a `System.Text.Json` bump was stale — it pins 10.0.10, which this repository already centralises on. - **`BinPickingCell` and `BinPickingClient` moved into `samples/Robotics/`**, which is where they belonged: the cell was already linking the arm, its kinematics and both USD assets out of `IntentEnabledRobot`. ## Upstream Work that belonged elsewhere went elsewhere rather than being worked around locally: - **`openusd-dotnet` [#13](https://github.com/marcschier/openusd-dotnet/pull/13)** — merged and shipped in `0.8.0-alpha`. `SilkFrameCapture.Capture` rendered only the first capture of a session and silently returned a blank frame for every one after it. Also filed [#14](https://github.com/marcschier/openusd-dotnet/issues/14) (no camera-by-prim API) and [#15](https://github.com/marcschier/openusd-dotnet/issues/15) (skipped tests reported as passed — which fooled me for several runs). Both are fixed; this branch takes `0.8.0-alpha` and deletes two workarounds, including 143 lines of hand-rolled projection maths. - **`opcua-drafts` [#66](https://github.com/marcschier/opcua-drafts/issues/66)–[#69](https://github.com/marcschier/opcua-drafts/issues/69)** — a close read of the spec produced 22 candidate ambiguities; 12 were discarded as misreadings with clause citations and the remaining 10 filed. One is concrete: five members exist in the NodeId CSV that no clause documents. - **`opcua-drafts` [#70](https://github.com/marcschier/opcua-drafts/issues/70)** — §9.5 could not express an empty observation or a false-positive retraction. **Adopted**, with the explicit-flag resolution argued for: `SubmitDetections` gained `SceneIsEmpty`, `SubmitCorrection` gained `RetractAll`, and the corrected-array rule relaxed from *exactly one* to *at most one*. - **`opcua-drafts` [#71](https://github.com/marcschier/opcua-drafts/issues/71)** — the Vision NodeSet browse-named all 13 `InputArguments` / `OutputArguments` Properties in the Vision namespace rather than the base namespace, so a stack could not find them and every Method in the specification was uncallable. **Fixed**; this branch now tracks the corrected artefact. ## The defect underneath it all The end-to-end suite was removed earlier in this branch because nine of its fourteen tests failed on one symptom: a client could not open a pipeline's `Feedback` object or enumerate its `Results`, even though browsing the same parent listed both. Restoring it meant explaining that, and the explanation was not in Vision. `RelativePathElement` initialised **`IsInverse` to `true`**. A caller that sets a `ReferenceTypeId` and a `TargetName` — which reads as *"the child reached by this Reference"* — was therefore asking the Server for the **inverse** Reference, and got `Bad_NoMatch` with nothing in the request to suggest why. `ObjectTypeClient.ResolveChildNodeIdAsync` is such a caller, and it is what **every source-generated Optional-child accessor in the stack** is built on — the accessor whose own documentation cites `AlarmConditionTypeClient.GetShelvingStateAsync` as an example. Vision is merely where it surfaced, because Vision puts two Optional children on the path an off-Server agent must take. The sibling helper in `StateMachineTypeClientExtensions` already set `IsInverse = false` by hand, which is the shape of a trap rather than a default. The default is now forward, `ResolveChildNodeIdAsync` says so explicitly, and the existing test that had recorded `true` as *expected* now pins `false` with the reasoning. Of the forty `RelativePathElement` construction sites in the tree, that accessor was **the only one relying on the default at all** — every deliberate caller already set it. Two further defects were found underneath it, both real and both fixed: - The generated `CreateOrReplace` helpers construct a child state object directly and leave **`TypeDefinitionId`** unset as well as `ReferenceTypeId`. A child referenced by nothing cannot be browsed; one with no type definition is a malformed Object that a client filtering by type silently skips — which is exactly what `EnumerateResultsAsync` does. Normalisation moved to `VisionNodeManager`, so it also covers the case the builder cannot see: a result published **at runtime** by an inference provider, long after the address space was built. That path produced results a client could list but not read. - `Opc.Ua.AI` shipped without an `AssemblyInfo`, so the solution built with six `CA1014` warnings. I had initially "fixed" this in the Vision client with Browse-based fallbacks, on the strength of a probe that appeared to show `TranslateBrowsePathsToNodeIds` failing even for `Server → ServerStatus`. That probe was wrong: it used an unqualified `ReferenceTypeIds`, which inside a `Opc.Ua.Vision.*` namespace binds to *Vision's* `ReferenceTypeIds` and yields a nonsense NodeId. Chasing that to the end is what turned up the real cause. **The fallbacks are gone** — they would have hidden a stack-wide bug behind a Vision-shaped workaround, and one of them matched on browse name while ignoring the namespace index. `tests/Opc.Ua.Vision.Intent.Tests` is restored and back in `UA.slnx`, passing **14/14** on its own merit rather than through a client-side workaround. A registration test now pins the pipeline's `Feedback` and `Results` children, which nothing covered before. ## Running it, and what that found The demo's premise is that a language model **looks at the bin and reasons about what it sees**. Running it end to end showed it could not, for reasons no test covered, because every one of them lived between components. **The sample did not compile.** Two `CS8604` nullable errors with `TreatWarningsAsErrors`, so neither the cell server nor the demo could be started at all. Worth recording that my first explanation was wrong: CI *does* build this project - the Linux leg's `net10.0` pass compiles it, log-confirmed - so this is not a coverage gap in the workflow, and no workflow change was made. **`Pick` and `Place` never moved the arm.** They were a delay plus a gripper action, so the operations reported `Succeeded` while nothing travelled. They now solve IK once and interpolate in joint space. **And the parts never moved**: the arm swung but the bin stayed full, because nothing tied the held object to the world state. The connector now creates 11 monitored items where it created 6, and a scripted run delivers `SeqNo=151` across 10 messages where it delivered `SeqNo=3` across 1. **The frame was smuggled through a string field.** The media provider put a base64 *data URI* of the PNG into `VisionImageReferenceDataType.Uri` - a field meant to be a *reference*, while the bytes were already returned in the inline `ByteString`. Every response therefore shipped the image twice, and a 1,311,400-byte PNG became a 1,748,536-character string against a `MaxStringLength` of 65,535: **26.7x over**. That is why raising `MaxByteStringLength`, `MaxArrayLength` and `MaxMessageSize` never helped - none of them govern a `String`. Separately the MCP tool assigned raw bytes where `ImageContentBlock` requires base64, so the wire carried `"data":"�PNG..."` - every non-UTF-8 byte replaced by U+FFFD, destroying the image and inflating the response to 6.8 MB. **The camera was aimed at nothing.** The arm's home pose, the flange scan pose the Vision model declared and the joint angles in the USD stage were three independent claims about where the camera was, and they disagreed - the declared flange orientation pointed the view 86 degrees away from the projection camera. Solving them as one thing exposed two further constraints that are easy to miss: the camera has to sit off the tool axis or it photographs its own gripper, and the IK branch has to be elbow-back or a link parks under the camera and fills the frame with the arm's own upper arm. A third was a regression this branch introduced and `Opc.Ua.Robotics.Tests` caught - aiming a straight-down camera from a point on the base's own X-Z plane puts the wrist on the J4/J6 singularity, so every motion away from home fails to solve. A 15-degree camera roll, free because the camera still looks straight down, gets 25 degrees clear, which is the margin the poses before this branch had. **The frame and the detections were different images.** The sensor declared 2448x2048, the clip endpoint 1280x1024, and the renderer produced 640x512 - not even the same aspect ratio. The ground-truth detector projects through the declared intrinsics, so its boxes lived in 2448x2048 space while an agent was handed a 640x512 picture: *"pick the red cube you can see"* arrived with coordinates that pointed off the image. Everything now agrees at 612x512, an exact 4x4 bin of the native sensor with the intrinsics scaled to match; the native size stays in the model and serial number, where it describes the hardware rather than the image. **`LatestClip` was created and never written**, so it reported `Bad_NoDataAvailable` for the life of the Server and a consumer following the model - read the published frame, call the method only if there is none - never got a frame. The dispatcher now publishes a clip it has just encoded, for every Vision server rather than this sample alone. One genuine API gap turned up on the way: `OpcUaServerOptions` exposes `MaxByteStringLength`, `MaxArrayLength` and `MaxMessageSize` and **no `MaxStringLength`**, so a Server that legitimately needs a long string could not raise the limit through the hosting API at all. Added, with tests. It is not the fix for anything above - that was a design bug - but the missing knob was real. ## Not finished - **Two projects sit below the repository's 80% line coverage.** `Opc.Ua.Vision.OpenUsd` is at 71.1% — what is left only executes with a real OpenUSD stage and plugin tree, which is not on every build agent. `Opc.Ua.AI.Inference` is now at **86.7%**, up from 12.9%: the code this branch *wrote* is covered by 25 tests, and `RestChatCompletionsBackend` — previously the whole of the shortfall — is covered by 18 more against a stubbed `HttpMessageHandler`, exercising success, refusal, throttling, retry-after, timeout and cancellation without a network. Everything else clears the bar — `Opc.Ua.Vision.Server` 92.1%, `Opc.Ua.AI` 91.4%, `.Vision.Client` 88.1%, `Opc.Ua.Vision` 85.7%, `Opc.Ua.AI.Server` 80.1%, all measured with coverlet. The CI coverage gate reports itself as advisory and non-blocking. - **`SamplesCollected` is not counted here, and `docs/Vision.md` now says so.** Section 9.4 requires a Server to count a negative example (`SceneIsEmpty` / `RetractAll` carrying a `GroundTruthLabel`) exactly as it counts one carrying geometry. `VisionNodeManager` does not, and structurally cannot: `SamplesCollected` is a property of `LearningJobType`, which the *AI Model Management* companion defines, and Vision reaches the job through a `NodeId` value rather than a Reference **precisely so this model takes no dependency on the model that defines it**. What the Server does guarantee is that the negative example survives the hop intact — both flags are carried verbatim to `IVisionFeedbackSink` — so a host that binds a learning job has everything it needs to satisfy the clause on the counter it owns. Stated in the limitations rather than left implied. ## Related Issues Was stacked on #4195, now merged. No tracking issue yet for the Vision implementation itself — happy to open one if you would like the design recorded as an ADR before this goes further. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. — *Types 8562, Server 4072, Client 2123, InformationModel 1063, Robotics 540, Tools 466, Vision 442, AI 74 and the restored Vision+Intent end-to-end suite 14/14, all green on net10.0. Only `.OpenUsd` (71.1%) is below the 80% bar — see Not finished.* - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. — *`dotnet build UA.slnx -c Release`: 0 errors, 0 warnings.* - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. — *net48: Types 8555, Client 2126, Vision 385 green. Two net48 Server tests fail (`CreateUserManagementSeamBindsTheModelToTheServerAsync`, and `DurableDataValueQueueVerifyReferenceBatchingAsync` only under parallel load); both were confirmed **pre-existing** by re-running them with the `IsInverse` change reverted, which fails identically.* - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. — *The net48 build legs failed because the restored end-to-end project used `ValueTask.FromResult` / `string.Create`, which do not exist on .NET Framework; fixed in `fb57522da` and verified building on every TFM. `DisposeDrainsHeldConnectionCallbackBeforeListenerDisposal` failed once on the Ubuntu Client leg — a 13 ms timing-sensitive reverse-connect disposal test that passes locally 2123/2123 and touches no browse paths.* - [ ] I have addressed **all** PR feedback received. ## Typed Vision-guided Robotics MCP workflows The branch now also makes the demo workflow first-class in the Full MCP profile: - replaces stringified Robotics intent and mission inputs with typed request schemas and exact name-or-NodeId selectors; - adds bounded operation/mission paging, stable step-to-operation identity, retained mission discovery, and `robotics_wait_mission`; - makes `vision_run_inference` return bounded typed detection, inspection, or segmentation summaries in one call; - adds same-session `robotics_vision_pick` to select a detection and submit Pick or Pick/Place work without implicitly taking authority, retrying, cancelling, or waiting; and - starts Publish workers for classic subscriptions so an independently launched OpenUSD viewer receives live robot and part updates. The migration guide documents the intentionally breaking MCP request-schema changes. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dab1302-19c5-4a9b-a50c-d97d389713aa Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b Copilot-Session: 99d1ab42-f83a-41ab-bf3e-85bf81fadace Copilot-Session: bcbc8ac9-5abd-42a8-8bac-e10533790e2e	2026-08-29 19:34:15 +02:00
romanett	e61ad4b306	Fully support sub-state machines in the fluent state-machine builder (#4322) ... # Description Part 16 §B.3 places `HasSubStateMachine` on the parent **state** node, but the fluent builder attached it to the state-machine **root** because `FluentFiniteStateMachineState` never materialized state nodes at all. Investigating #3821 showed the gap was wider than the reference placement: 1. **No state nodes existed.** The NodeId written into `CurrentState/Id` — `new NodeId(stateId, ElementNamespaceIndex)` — was **dangling**: no node with that id was in the address space. 2. **`AvailableStates` was never populated**, so `GetAvailableStatesAsync` returned empty against a fluent-built server. 3. **The whole client discovery chain was therefore dead.** `ObserveEffectiveStateAsync` enumerates `AvailableStates` and browses `HasSubStateMachine` from each state node — it found nothing. The client side was already spec-correct; only the server deviated. 4. **`HasSubStateMachine` is non-hierarchical** (a subtype of `NonHierarchicalReferences`, `i=32`). Using it as the sub-SM's `BaseInstanceState.ReferenceTypeId` left the sub-SM unreachable from a hierarchical browse of the machine. ## What changed **Element nodes are materialized.** Definition-mode machines now create one `StateType` node per declared state and one `TransitionType` node per declared transition — each a `HasComponent` child carrying `StateNumber` / `TransitionNumber`, with the §B.4 `FromState` / `ToState` / `HasEffect` references and `HasCause` added by `WithCause`. `AvailableStates`, `AvailableTransitions` and `LastTransition` are populated, so `CurrentState/Id` and `LastTransition/Id` resolve to real, browsable nodes. NodeIds are per-instance, so several machines built from one definition coexist. **`HasSubStateMachine` moved to the state node**, with the inverse `SubStateMachineOf`, and the sub-SM is attached as a `HasComponent` child (fixing 4). **Core gains element-id mapping hooks.** `GetCurrentStateId` / `UpdateStateVariable` / `UpdateTransitionVariable` hard-coded the numeric form and were non-virtual, so a subclass could not redirect them. They now route through `public virtual` `GetStateNodeId` / `GetStateId` / `GetTransitionNodeId` / `GetTransitionId`, whose defaults reproduce the previous `(elementId, ElementNamespaceIndex)` convention **exactly** — every stack-shipped and generator-emitted machine is unaffected, and there is a regression test asserting that. **Generated machines report their model namespace.** The generator now emits an `ElementNamespaceUri` override on `FiniteStateMachineType` subtypes, taken from the nearest type that actually *declares* state children — a subtype that only adds behaviour inherits its elements, and their namespace, from its base. **Ordering and validation fixes.** `WithSubStateMachine` and `WithInitialState` are now order-independent (previously, attaching a sub-SM before setting the initial state left the child suspended until the parent left and re-entered the state — silently). `WithSubStateMachine` validates `parentStateId`, and the builder rejects duplicate element browse names, since element NodeIds are now derived from them. ## A defect this surfaced Code that assembles nodes with the generated `CreateInstanceOf<Type>` factories never calls `NodeState.CreateAsPredefinedNode`, so `OnBeforeCreate` / `OnAfterCreate` are skipped for the entire subtree — and `OnAfterCreate` is the only place a state machine resolves its element namespace. It is also where `ConditionState` binds its `Enable` / `Disable` / `AddComment` handlers and `AlarmConditionState` binds the shelving methods, so the impact is broader than state machines. This PR fixes only the two call sites it depends on — DI's `SoftwareUpdateFacetWiring` and Robotics' `RoboticsOperationBuilder`. **The repository-wide audit is deliberately left out of scope and tracked in #4320** (24 files call these factories; none completed the lifecycle). The Robotics regression had no test coverage at all, so an assertion on `CurrentState/Id` was added and verified to fail without the fix. ## Breaking changes - Sub-state machines are **no longer browsable from the state-machine root**. Callers passing a machine's `ObjectId` to `GetSubStateMachineAsync` must pass a state NodeId instead — from `GetAvailableStatesAsync` or a snapshot's `CurrentStateId`. - `CurrentState/Id` for fluent machines changes from the numeric form to the materialized node's id. Code reading a numeric id out of it should call `FiniteStateMachineState.GetStateId` rather than parsing the identifier. - Generated machines qualify `CurrentState/Id` / `LastTransition/Id` with their companion-spec namespace instead of the OPC UA one. Documented in `docs/WhatsNewIn2.0.md` and `docs/StateMachines.md`. ## Notes for reviewers - Adding `virtual` to the `FiniteStateMachineState` members is a binary (not source) breaking change — assumed acceptable on the 2.0 line, but worth confirming against the compatibility policy. - Address-space cost is one node plus one property per declared state and transition, per machine instance; called out in the docs for servers instantiating many machines. - `AvailableTransitions` is modelled as a `HasComponent` `BaseDataVariable` (matching the standard NodeSet and what the client's browse path expects), not a Property. ## Related Issues - Fixes #3821 - Surfaced #4320 (node create lifecycle audit — not addressed here) ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Test runs (net10.0) | Suite | Result | |---|---| | `Opc.Ua.Server.Tests` (full) | 4734 passed, 5 skipped | | `Opc.Ua.SourceGeneration.Core.Tests` | 3780 passed | | `Opc.Ua.SourceGeneration.Tests` | 132 passed | | `Opc.Ua.SourceGeneration.Stack.Tests` | 94 passed | | `Opc.Ua.Types.Tests` | 8763 passed | | `Opc.Ua.Robotics.Tests` | 540 passed | | `Opc.Ua.Robotics.Intent.Tests` | 22 passed | | `Opc.Ua.Di.Tests` | 424 passed | | `Opc.Ua.ISA95.Tests` | 136 passed | | `Opc.Ua.Aas.Tests` | 670 passed | | `Opc.Ua.Positioning.Tests` | 69 passed | | `Opc.Ua.Client.Tests` (StateMachines) | 36 passed | Only net10.0 was run locally; the .NET Framework leg is left to CI, hence the unchecked box above. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>	2026-08-29 13:33:16 +00:00
romanett	41db5aca60	Fluent: publish Configure-created nodes' references on foreign-manager nodes (Objects folder placement) (#4331) ... # Description A node created inside the fluent `Configure` hook (source-generated `[NodeManager]` partial, or the hosting `AddNodeManager(uri, build)` route) had no way to appear below the Objects folder — or below any node owned by another node manager. `AsyncCustomNodeManager.AddReverseReferencesAsync` mirrors inverse references into the `externalReferences` dictionary **before** the `Configure` partials run, and the fluent surface never sees the dictionary, so a configure-created node with an inverse `Organizes` reference to `ObjectsFolder` existed and simulated but never showed up under Objects. This implements the three coordinated pieces proposed in #4329, plus the companion namespace gap: **1. Idempotent mirroring pass** (independently worthwhile hardening) - `AddExternalReference` in `AsyncCustomNodeManager` and `CustomNodeManager2` now has if-missing semantics (linear scan of the per-source list). - `TypeTable.AddEncoding` no longer appends a duplicate encoding id; re-registering the same encoding is a succeeding no-op. (`AddRootNotifierAsync` and the node-side `AddReferenceIfMissing` branch were already idempotent.) **2. Re-run the pass after `Configure`** - New protected `FluentNodeManagerBase.CompleteConfigureAsync(externalReferences, ct)` wraps `AddReverseReferencesAsync` so hand-written managers using `CreateFluentBuilder` benefit too. - The generator-emitted `CreateAddressSpaceAsync` and the hosting `FluentNodeManager` invoke it once between the `Configure` callbacks and `Seal()`. Timing is safe: the master distributes `externalReferences` only after every manager's `CreateAddressSpaceAsync` returns. Side benefit: inverse `HasNotifier` references on configure-created event sources get root-notifier registration for free. - The hosting `FluentNodeManager`'s hand-rolled `externalReferences` workaround for its root folder is deleted — the root's inverse `Organizes` reference now flows through the shared pass (mirrored exactly once, covered by a test). **3. Discoverable fluent sugar** - `OrganizedBy(parentId)` / `UnderObjectsFolder()` on `INodeBuilder` write the inverse `Organizes` reference; with piece 2, placement is just "write the inverse reference". - A parentless `CreateInstance<TState>` on `INodeManagerBuilder` takes a constructor-style factory (`p => new BoilerState(p)`, keeping the surface reflection-free and AOT-safe), materializes the subtree from the type model via `NodeState.Create`, rebases all NodeIds through the manager's `INodeIdFactory` (`AssignInstanceNodeId`/`AssignInstanceChildNodeIds` — the same pair the generated factories use, so declaration-id children cannot collide with the type model), and registers via `AddPredefinedNodeSynchronously`. The Boiler #2 shape becomes fully fluent: ```csharp builder.CreateInstance( new QualifiedName("Boiler #2", NamespaceIndexes[1]), p => new BoilerState(p)) .Configure(n => n.UnderObjectsFolder()); ``` **Companion gap, same theme:** `[NodeManager]` grows `AdditionalNamespaceUris`, flowing through attribute discovery → binding → generator so the generated constructor reports a second (instance) namespace at construction and the generated factory advertises it in `NamespacesUris` — today `SetNamespaces` after construction is not enough because `MasterNodeManager` builds its namespace routing from what the manager reported when it was built. **Scope boundary** (as discussed in the issue): startup-time configuration only; for nodes created after startup the correct primitive remains `IMasterNodeManager.AddReferencesAsync`. **Reviewer notes** - The generated-code change was validated against the real pipeline: a forced fresh compile of `Opc.Ua.Server.Tests` (whose CoverageNodeSet managers are `[NodeManager]`-generated) emits `await CompleteConfigureAsync(externalReferences, cancellationToken)` between `Configure` and `Seal()` and compiles against the real base class. - The distribution consumer (`AddReferencesAsync`) already used `AddReferenceIfMissing`, so the whole chain is defense-in-depth idempotent. - 15 new tests: double-run idempotence, second-pass pickup of late-registered nodes, the new reference sugar, root-instance creation incl. child-id rebasing and reference remapping, hosting end-to-end Objects-folder placement, emitted-sequence and `AdditionalNamespaceUris` generator tests, and a full Roslyn-pipeline attribute test. Docs updated in `docs/NodeManagers.md` (new "Creating nodes under other managers' nodes" section, attribute docs, fluent-surface sections). - Verified locally: all touched projects build with 0 warnings; full `Opc.Ua.Server.Tests` (4,725 passed / 0 failed / 5 skipped, net9.0), `TypeTableTests` (113), `Opc.Ua.SourceGeneration.Core.Tests` (41), `Opc.Ua.SourceGeneration.Tests` (132), and the Quickstarts.Servers sample all pass. ## Related Issues - Fixes #4329 - Motivating case: OPCFoundation/UA-.NETStandard-Samples#756 / OPCFoundation/UA-.NETStandard-Samples#795 (Workshop/Boiler migration) ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-29 10:38:56 +02:00
romanett	24399f1808	Fix source generator MODELGEN003 diagnostic dropping message placeholders (#4310) ... ## Problem `LoggerAdapter.Log<TState>` in `Tools/Opc.Ua.SourceGeneration/SourceGeneratorTelemetry.cs` reported an **already fully-formatted** logger `message` against a looked-up `DiagnosticDescriptor` whose `messageFormat` may declare more positional placeholders than the single formatted string carries. For `EventId == 3` the descriptor is `SourceGenerator.Exception` (`MODELGEN003`) with: ``` messageFormat = "Exception during model generation '{0}': {1}" ``` That format has **two** placeholders but only **one** argument (`message`) was supplied. Roslyn's `Diagnostic.GetMessage()` runs `string.Format(format, args)`, which throws `FormatException` because `{1}` has no argument; Roslyn's internal catch then returns the **raw, unsubstituted template**. The user therefore saw the literal text `Exception during model generation '{0}': {1}` with the placeholders unreplaced, masking the real error. ## Fix Route the pre-formatted message through a new `CreateFormattedDiagnostic` helper that reports it via a **passthrough descriptor whose `messageFormat` is exactly `"{0}"`**, while preserving the original descriptor's `Id`, `Title`, `Category`, `DefaultSeverity`, `IsEnabledByDefault`, `HelpLinkUri` and custom tags: ```csharp public static Diagnostic CreateFormattedDiagnostic( DiagnosticDescriptor descriptor, string message) { var passthrough = new DiagnosticDescriptor( descriptor.Id, descriptor.Title, "{0}", descriptor.Category, descriptor.DefaultSeverity, descriptor.IsEnabledByDefault, helpLinkUri: descriptor.HelpLinkUri, customTags: descriptor.CustomTags.ToArray()); return Diagnostic.Create(passthrough, Location.None, message); } ``` `LoggerAdapter.Log` now calls this helper instead of `Diagnostic.Create(descriptor, Location.None, message)`. This renders the message verbatim under the correct diagnostic id regardless of the descriptor's placeholder arity, and is safe even when the message itself contains `{`/`}` characters (the format string is only `"{0}"`). > Note: the public `descriptor.CustomTags` (`IEnumerable<string>`) + `.ToArray()` is used because Roslyn's `ImmutableCustomTags` property is `internal`. ### Why the helper lives in `SourceGeneratorHelpers.cs` (not `SourceGenerator.cs`) `SourceGeneratorTelemetry.cs` and `SourceGeneratorHelpers.cs` are shared (glob-linked) into **both** the model generator (`Opc.Ua.SourceGeneration`) and the stack generator (`Opc.Ua.SourceGeneration.Stack`). The stack generator has its **own** `SourceGenerator` partial class and does **not** compile the model generator's `SourceGenerator.cs`. Placing the helper in `SourceGenerator.cs` broke the stack build with `CS0117: 'SourceGenerator' does not contain a definition for 'CreateFormattedDiagnostic'`. `SourceGeneratorHelpers.cs` is the shared partial (where `Guard` also lives), so the helper is available to both generators. ### Guard path untouched The other emission path, `SourceGenerator.Guard(...)` in `SourceGeneratorHelpers.cs`, uses the same `Exception` descriptor but correctly passes **two** args (`ex.Message`, `ex.ToString()`), so that path was already fine and is **not** changed. ## Tests New `tests/Opc.Ua.SourceGeneration.Tests/SourceGeneratorTelemetryTests.cs` (4 tests): 1. **`DirectReportAgainstMultiPlaceholderDescriptorLosesMessage`** — documents the pre-fix behaviour: the raw `Diagnostic.Create(Exception, ..., "boom")` render drops `"boom"` and leaks `{1}`. 2. **`CreateFormattedDiagnosticRendersMessageVerbatim`** — the fix renders `"boom"` exactly, with no `{0}`/`{1}`. 3. **`CreateFormattedDiagnosticPreservesDescriptorMetadata`** — id (`MODELGEN003`), severity, category, help link, IsEnabledByDefault and custom tags are preserved. 4. **`CreateFormattedDiagnosticIsSafeForBracesInMessage`** — a message containing `{`/`}` is emitted verbatim. ## Validation - `tools/Opc.Ua.SourceGeneration` (and the stack variant) build with **0 warnings / 0 errors**. - Full `Opc.Ua.SourceGeneration.Tests` suite on net10.0: **132/132 passing**, including the 4 new tests. ## Files changed - `tools/Opc.Ua.SourceGeneration/SourceGeneratorHelpers.cs` — added `using System.Linq;` + `CreateFormattedDiagnostic` helper. - `tools/Opc.Ua.SourceGeneration/SourceGeneratorTelemetry.cs` — `LoggerAdapter.Log` calls the helper. - `tests/Opc.Ua.SourceGeneration.Tests/SourceGeneratorTelemetryTests.cs` — new regression tests. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>	2026-08-24 15:19:26 +00:00
Marc Schier	06ae192084	Fix CI failures tracked by #4299 (#4308) ... ## Summary - stabilize the History, authentication lockout, Robotics, redundancy, and DI tests reported in #4301-#4305 - fix the closed-transport buffered-write completion deadlock that hung `ConnectMultipleSessionsAsync` - add macOS crash diagnostics for #4306 and retain evidence for root-cause analysis - run macOS tests and NativeAOT in GitHub Actions pull requests, using supported Intel and ARM64 runners, while keeping every Azure macOS definition disabled by default - move all remaining source-level `InternalsVisibleTo` declarations into project files so signed OpenUSD builds receive the repository public key ## Local validation - MCP PubSub: 29 passed - History focused tests: 3 passed - Sessions lockout: 3 passed - Sessions multiple-session regression: 3 passed - deterministic binary client channel tests: 22 passed - Robotics runtime: 34 passed - Redundancy focused tests: 4 passed - OpenUSD net10.0: 914 passed - affected OpenUSD projects build on net10.0 and net48 Refs #4299, #4300, #4306 Fixes #4301 Fixes #4302 Fixes #4303 Fixes #4304 Fixes #4305 --------- Copilot-Session: 7cb486de-8b4c-43c5-a900-e7d2e0ad04a8	2026-08-22 19:30:19 +00:00
Copilot	d525bc3f28	Bound reconnect cleanup and organize samples by domain (#4276) ... # Description This PR fixes bounded ManagedSession reconnect cleanup and reorganizes the sample applications into domain-focused folders. ## Reconnect cleanup - Bounds reconnect integration cleanup with independent 30-second close and dispose limits. - Sends `CloseSession` through the awaited transport request path during `Session.DisposeAsync`. - Preserves explicit `Session.CloseAsync` behavior that waits for or cancels outstanding publish requests. - Covers the disposal request path with a regression test. ## Sample organization - `samples/OpenUsd`: `GeneratorServer`, `SiteCompositionServer` - `samples/Redundancy`: `RedundantClient`, `RedundantServer`, `RedundantPubSub` - `samples/Isa95`: `MinimalIsa95Server` - `samples/MinimalApi`: `MinimalBoilerServer`, `MinimalCalcServer`, `MinimalClient` - `samples/DI`: `PumpDeviceIntegrationServer` - `samples/Lds`: `ConsoleLdsServer` - `samples/Reference`: `ConsoleReferenceClient`, `ConsoleReferenceServer` - `samples/PubSub`: `ConsoleReferencePubSubClient` `UA.slnx` now exposes the same requested domain groups. Project references, tests, runtime sample probes, CI workflows, Dockerfiles, Compose files, helper scripts, and documentation were updated to the new paths. `samples/Robotics/MinimalRobotServer` remains under `Robotics`. ## Validation - `dotnet build UA.slnx` - 3 Redundancy short-haul sample tests - 70 DI sample tests - 914 OpenUSD tests - 1 WotCon model-materialization test - 137 AOT tests - `docker compose config` for all 6 moved Redundancy Compose files ## Related Issues - Fixes #4273 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce new build or analyzer warnings. - [x] I built the complete `UA.slnx` solution locally. - [ ] I ran all tests locally against at least .NET Framework and .NET 10. - [ ] I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings. - [x] I have addressed all PR feedback received. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <mschier@microsoft.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Copilot-Session: a2c2838f-74ae-4997-a1d4-50574d9e0636	2026-08-21 04:35:47 +00:00
Marc Schier	397db603db	Add OPC UA for Asset Administration Shell: the published OPC 30270 and v3 draft (#4244) ... # Description Implements the **Asset Administration Shell** companion specifications on top of `marcschier/wot-type-binding`, in two halves that a Server chooses between: * **AAS V3** — the [draft](https://github.com/marcschier/opcua-drafts/tree/main/companion-specs/AAS) (`http://opcfoundation.org/UA/I4AAS/v3/`), all sixteen conformance units: the metamodel, the registry built on the abstract xRegistry base, generational materialization, federation, packages with digest-bound integrity, the Digital Product Passport vocabulary, and the Annex F bridge to WoT Thing Descriptions. * **AAS V2** — the published [OPC 30270](https://reference.opcfoundation.org/specs/OPC-30270/full) (`http://opcfoundation.org/UA/I4AAS/`), which maps the AAS V2.0.1 metamodel: object model, ingestion of XML, JSON and AASX, materialization, server and client. The two are different metamodel generations rather than two revisions of one model — V2 keeps `Asset` as a first-class Identifiable, `identification.idType`, Views and ordered collections, none of which survive into V3 — so they are **alternatives**. A Server hosts one or the other; this is documented rather than enforced at startup. Each is named for the generation it maps (`Opc.Ua.Aas.V2`, `Opc.Ua.Aas.V3`, `AddAasV2Server`, `AddAasV3Server`), and what genuinely serves both carries no marker and sits at the root of `Opc.Ua.Aas`: the deterministic NodeId encoding and idShortPath conventions, `AasOptional<T>`, the digest rule, the materialization diagnostics, the AASX container walk, and the value and operation providers, which are keyed on `NodeId` and `Variant` and so carry no version at all. ## What is in here **New libraries**: `Opc.Ua.Aas`, `Opc.Ua.Aas.Server`, `Opc.Ua.Aas.Client`, `Opc.Ua.Aas.WoT`, plus `samples/Aas` and `docs/Aas.md`. **Enabling changes to existing code**, each needed by the above: * The OPC UA `Decimal` DataType added to `Opc.Ua.Types`, with its Part 6 §5.1.10 wire encoding. `BinaryDecoder` gained one internal member, `TryReadRemainingBodyBytes`, because a trailing field with no self-describing length cannot otherwise be read; `Decimal.Decode` is its only caller. Part 6 Table 3 types the unscaled `Value` as an **OctetString whose byte count is `Length` − 2**, not a length-prefixed `ByteString`, so it carries no length of its own — `DecimalEncodingTests` pins the body byte-for-byte against that mistake, and against the `Length ≤ 2` values the same table calls invalid. * The xRegistry projection engine extracted into `Opc.Ua.XRegistry.Server` and `Opc.Ua.WotCon.Server` migrated onto it (−848/+387 lines), with the WoT Connectivity tests passing **unedited**. * Two source-generator fixes, both of which made a model impossible to build (see below). ## Defects found and fixed along the way Running the stack end to end, rather than trusting per-area test suites, surfaced defects that no unit test caught. The ones worth a maintainer's attention: * **The generator emitted non-terminating constructors.** A structure field with no declared default was initialized with `new T()`, so a self-referential DataType (`AASReferenceDataType.referredSemanticId`) recursed until the stack overflowed and `AddOpcUaAas` could not load the model at all. The test host *died* rather than failing. Only the field that closes a cycle is now left null. * **The generator omitted `new` on a redeclared placeholder.** `AASOrderedSubmodelElementCollectionType` redeclares the `SubmodelElement` slot of its base type, and the generated `Add` method hid the base one without saying so, so the V2 model would not compile. No existing model could have hit either defect — both are build failures, so anything that builds today is unaffected. * **The metamodel half had never worked outside its unit tests**: the emitted NodeSet declared no reference-type aliases and was rejected by the loader; the Mandatory `Invoke` Method was never emitted, because a NodeSet is imported as written rather than instantiated from its type; the runtime bound at the authored namespace index instead of the one the Server assigns; and `MethodDeclarationId` was addressed in the wrong namespace. Every materializer test inspected the in-memory object graph, which cannot see whether the graph is loadable — two tests now import it exactly as `RuntimeNodeSetNodeManager` does. * **The client could not see any Property member.** It browsed `HasComponent` with subtypes, but `HasProperty` is a sibling under `Aggregates`, so reading a value failed and invoking failed argument validation. * **Every repeated registry write forked a duplicate group and resource** instead of adding a version, because the identifier was re-derived against a sibling set that already contained the entity itself. This also made the persistence bounds unreachable. * `.gitignore`'s `**/packages/*` silently swallowed the whole package-integrity implementation and its tests, which would have vanished on a fresh clone. ## Specification findings Nine findings against the V3 draft are fixed in [marcschier/opcua-drafts#75](https://github.com/marcschier/opcua-drafts/pull/75). The substantive one: clause 6.1.5's absent-versus-empty rule was **unsatisfiable** for five container fields that materialize as parent components with no node of their own, which made `AAS-LosslessRoundTrip` unachievable. OPC 30270 contradicts itself twice. Both are recorded in `AasV2ConformanceUnits.cs` and the documentation rather than smoothed over: Table 83 spells the unit `I4AAS MultiLangaugeProperty` where Table 85 spells it correctly, and Table 85 lists an `I4AAS Security` unit that Table 83 never defines. Table 84, which would assign profile URIs, is empty, so `ServerProfileArray` gains no I4AAS entry. ## Known limitation Calling an `Operation` **with arguments** answers `BadTooManyArguments`. Method resolution works from both the instance NodeId and the `MethodDeclarationId`, and the argument definitions are published, but `NodeState.AddChild` does not route through `MethodState.FindChild`, so a Method imported from a NodeSet leaves its typed `InputArguments` null. Closing this means changing how the shared NodeSet import claims typed children, which affects every consumer of runtime NodeSets and is out of scope here. It is documented in `docs/Aas.md` §11, the conformance matrix marks `AAS-OperationInvoke` partial rather than claiming it, and `samples/Aas` exercises the path and reports the status rather than working around it. V2 is unaffected — its `Operation` is an OptionalPlaceholder with no declared arguments. ## Related Issues - No tracking issue yet. Opening as a **draft** for design discussion first, per the ADR guidance below; happy to open one and link it once the shape is agreed. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Verification | Check | Result | |---|---| | AAS tests | **670 passed, 0 failed** on net472, net48, net8.0, net9.0, net10.0 | | Coverage | Client 80.8 %, Server 81.2 %, `Opc.Ua.Aas` 82.0 %, WoT 82.4 % — all above the 80 % gate | | Libraries | `Opc.Ua.Aas`, `.Server`, `.Client`, `.WoT` build clean on every target framework, zero warnings | | Regression | `Opc.Ua.Types` 8677, `Opc.Ua.WotCon` 1094, `Opc.Ua.XRegistry` 198, `Opc.Ua.Server` 4050 — all green | | Sample | `samples/Aas` builds warning-free and runs end to end | | Merge validation | Solution builds clean on net10.0 and net48; WoT Types 504, WotCon 1111, AAS 670, XRegistry 198 tests passed | | CI | All 99 checks green on `ff9298e44`; only the CLA signature status is outstanding | ### Review pass A full review against both specifications, and for API consistency and correctness, produced twelve fixes. Each one has a test that fails without it. | Defect | Where | Why it mattered | |---|---|---| | V2 projection aborted on **every** `File` read from a document | `AasV2EnvironmentRuntime` | The runtime bound `File/Value` unconditionally, the materializer emitted it only with content, and the builder throws `BadNotFound` for an unknown NodeId — aborting the whole registration. No reader ever populates `AasFile.File`, so no AAS V2 document could be hosted at all. | | `aas-suppl` anchored on the wrong part | `AasxPackageReaderCore`, `AasxPackageWriter` | IDTA and the Package Explorer anchor it on the environment part; we used the origin part. Every real `.aasx` returned zero supplementary files, silently. Affects both halves. | | Federation failed closed on pooled connections | `AasFederation` | `ConnectCallback` fires only for new connections, so the second fetch to an origin — and all of net472/net48/netstandard2.1 — failed with a security-flavoured error. The peer is now validated *inside* the callback, which makes reuse safe by construction. | | Malformed `Decimal` escaped as `ArgumentOutOfRangeException` | `BinaryDecoder` | A declared body shorter than `Scale` produced a negative read length. Two of the three test cases had been passing for the wrong reason. | | V2 and V3 cross-wired their value providers | `OpcUaAasServerBuilderExtensions` | Both registered `IAasValueProvider`; whichever was added first served both AddressSpaces. V2 now has its own contract, so co-hosting is harmless rather than silently wrong. `EnvironmentFolder` was also bound and ignored on the V2 path. | | Export filter skipped three nesting kinds | `AasEnvironmentExporter` | Content under `Entity`, `AnnotatedRelationshipElement` and `Operation` was exported unevaluated *and* stamped with a Digest presenting it as complete (clause 6.5.10). List authorization paths also shifted after a denial. | | Concealment held at the Method but not in the projection | `AasRegistryModel` | `GetSubmodel` answered `BadNotFound` while an anonymous Browse revealed the node, its identifier, semanticId and digest (clause 6.5.7). | | File handles were unsynchronized and unowned | `AasElementFileManager` | The async node manager holds no lock, so concurrent Calls raced a bare `Dictionary`; sequential handles let any Session read, seek or commit another's. Writes were also unbounded. | | Projection host was unimplementable outside the stack | `AasEnvironmentProjection` | The handle demanded a type with an internal constructor — our own tests used `FormatterServices.GetUninitializedObject` to get round it. Now mirrors WotCon's interface-typed registration. | | Embedded `FileType` omitted 7 of 10 Mandatory members | `AasEnvironmentMaterializer` (V2) | OPC 10000-3 6.4.4 requires them all; without `Size` and `OpenCount` a Client cannot size a read or detect an abandoned handle. | | Round-trip oracle ignored most of the model | `AasRoundTripComparer` | `Blob`, `File`, `ReferenceElement` and `BasicEventElement` had no case arm, so their fields compared equal by falling off the switch; `valueType`, `semanticId`, `entityType` and `Key.type` were never read. The clause 6.4 losslessness claim rested on it. | Known, unchanged: calling an `Operation` with arguments answers `BadTooManyArguments`, because `NodeState.AddChild` does not route through `MethodState.FindChild`. Documented in `docs/Aas.md` §11. Not addressed in this pass, and worth a maintainer decision: OPC 30270 §9.2 says all NodeIds of Nodes not defined in the companion specification **shall not** use the standard namespaces, and Table 87 marks a vendor-specific instance namespace Mandatory. Both halves currently mint instances in the companion namespace. Separating them touches NodeId minting, the runtime and the client together, so it is better done deliberately than folded into a review pass. This is a large change and touches shared code (`Opc.Ua.Types`, the source generator, the xRegistry projection). It is deliberately a draft: the enabling changes are separable if maintainers would prefer them split into their own PRs. The latest `master` is merged into this branch in `0dd0ff0e1`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 Copilot-Session: c3d88950-4a65-4ff6-87d2-1f545a875c9b Copilot-Session: b8ab3763-b9bd-46ae-b0ef-045db26e5782	2026-08-18 08:11:47 -07:00
romanett	333de8305b	Add kitchen-sink coverage NodeSet2 with three load pipelines (#4251) (#4272) ... # Description Regression fence for the NodeSet2 import + code-generation surface. This adds a single, purpose-authored "kitchen-sink" NodeSet2 (`Opc.Ua.CoverageTest.NodeSet2.xml`) plus a small dependent secondary model that deliberately flip a broad set of the knobs defined by `UANodeSet.xsd` (every node class, every `Definition` shape, custom reference types, all five modelling rules, scalar/array/matrix values, a deep branching instance tree, and a wide set of reference relations). The same bytes are then loaded into **real** servers (`ServerFixture<T>` + `ReferenceServer`) through three independent pipelines and validated node-by-node, attribute-by-attribute, reference-by-reference: 1. **Source-generation, hand-written manager** - the XML is consumed at compile time via `<AdditionalFiles ModelSourceGeneratorPrefix="Opc.Ua.CoverageTest">`; the generated `AddOpcUaCoverageTest(context)` composes the predefined nodes inside a `CustomNodeManager2`, so every node materialises as its generated `NodeState` subclass. 2. **Runtime import** - the identical XML is imported at startup through `RuntimeNodeSetNodeManagerFactory` / `AddRuntimeNodeSet`. 3. **Source-generation, generated fluent manager** - a `[NodeManager]`-attributed partial makes the generator emit the whole `FluentNodeManagerBase` manager, factory and typed fluent-builder surface. A shared, data-driven `[TestCaseSource]` battery runs the identical assertions against all three servers, and a cross-pipeline equivalence fixture asserts the three address spaces agree. A dependent **second namespace** (`.../Secondary/`) exercises cross-namespace `HasTypeDefinition`, `HasSubtype` and `HasComponent` references in every pipeline. Authoring the model surfaced two latent source-generator bugs, both fixed here: - `NodeSetToModelDesign` crashed on a top-level `<ServerUris>` table (`StringTable.Append(null)` for the reserved local-server slot). Added a null guard plus a focused regression test (`NodesetServerUrisTests`). - `FluentBuilderGenerator` emitted colliding typed-builder accessors for any model with more than one structure encoding, because `DataTypeEncoding` objects hang off their `DataType` and surfaced as parent-less "root" instances sharing the reserved `DefaultBinary`/`DefaultXml`/`DefaultJson` symbolic names. They are encoding metadata, never fluent-wired, so they are now excluded from the typed builder. A few edges are intentionally not asserted for cross-pipeline equivalence because the pipelines legitimately differ (documented in `CoverageNodeSet/README.md`): type-dictionary / `DataTypeDescription` nodes (the generator regenerates schema rather than emitting them), a handful of ambiguous duplicate-hierarchical / View-`Organizes` edges, and runtime structure stand-ins (structure definitions are validated via the `DataTypeDefinition` round-trip instead). ## Related Issues - Fixes #4251 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>	2026-08-17 14:59:58 -07:00
Marc Schier	aad8350bc1	Pluggable symmetric cryptography, an awaitable asymmetric path, registrable security policies, and PubSub crypto offboarding (#4206, #4207, #4208, #4210) (#4232) ... # Description Closes the three crypto-offboarding follow-ups split out of #4192. Each was recorded there as knowingly *not* done, with the reason and what would justify revisiting it. Those conditions are met, and where one of them turned out to be unachievable that is written down rather than worked around. The safety property throughout: **every seam is opt-in and null-checked, and in the default software configuration the added code is observably inert.** No interface dispatch on the per-message path, and the asynchronous paths complete synchronously — so channel locking order, buffer ownership and error sequencing are unchanged for a deployment that configures nothing. ## #4207 — symmetric, key-derivation and RNG provider seams `ISymmetricCryptoProvider`, `IKeyDerivationProvider` and `ISecureRandomSource`, added on the terms #4207 set: only once a concrete requirement arrived (a validated module that must perform *every* operation, not just the asymmetric ones), behind a fast path, with `SymmetricChannelCryptoBenchmarks` as the gate. They are **facets discovered by type test**, not members of `ICryptoProvider`, so providers written against the shipped interface still compile. `PlatformCryptoProvider` implements all three, which answers the "an interface nothing implements" objection recorded in the issue. `CryptoProviderFacets` returns `null` both when no registry is configured *and* when resolution lands on the platform, because the platform facets are the inline code. Resolution happens once, in `CalculateSymmetricKeySizes`, and is held for the life of the channel. `CryptoCompliance.GetUnservedOperationPurposes` closes the failure the seam would otherwise introduce: a provider bound to `ChannelSymmetric` without implementing the facet is silently replaced by the platform, so a deployment could believe its validated module performed the per-message cryptography while the platform did. Under `FipsOnly` that now refuses to start. ## #4208 — the asymmetric path is awaitable `RSA` and `ECDsa` are .NET's contracts and cannot be replaced without making every ready-made hardware and cloud implementation unusable. Instead an implementation may **also** declare `IAsyncRsaKey` / `IAsyncEcdsaKey`, which the stack finds by type test. Now asynchronous: the secure channel open and renew path, user identity token signing and decryption, and session activation. A software key declares neither facet, so those paths complete synchronously and nothing about their ordering changes. `ReadAsymmetricMessage` is refactored into shared helpers rather than duplicated — adding an `await` was not worth two hundred cloned lines of signature, padding and endpoint-selection logic. `AsymmetricMessage` / `AsymmetricWriteResult` exist because an asynchronous method cannot have `out` parameters. The channel no longer serialises its state on a monitor. A monitor cannot be held across an `await`, and a plain `SemaphoreSlim` was not an option at the time: the code relied on re-entrancy in eight places (`HandleIncomingMessage` holds the lock and calls `ForceChannelFault`, which takes it again; `ForceReconnect` and `Shutdown` do the same). `ChannelGate` replaces it at 32 sites and `DataLock` is `[Obsolete]`. That reliance has since been removed as well — see [Removing the gate's re-entrancy](#removing-the-gates-re-entrancy) below. ### Defects this surfaced, all fixed and regression-tested **Detached work started inline.** Writes were started with `_ = WriteBuffersAsync(...)`. An async method runs its prologue on the caller's stack, so `LeaveInheritedContext` — which exists to stop *detached* work inheriting the right to re-enter the gate — was stripping the **caller's own** entitlement; when the send then completed synchronously, the completion blocked on a gate that very thread was holding. The client sent its OpenSecureChannel request and never processed the response. Writes are now queued. **A synchronous gate handle held across an `await`.** `Enter()` records the acquiring thread so an inline completion callback can re-enter — `ChannelAsyncOperation` invokes its callback both inline and detached. That record is only sound while the holder is synchronously on that thread; the token renewal path held it across an await, so a reused pool thread was recognised as the holder. **`SignAsync` rejected the policy that signs nothing.** It validated the certificate before the algorithm, so `SecurityPolicies.None` — which carries no certificate — threw where the synchronous `Sign` returns `null`. **`EnterAsync` lost the holder when contended** *(found in review)*. The `AsyncLocal` recording ownership was written inside the awaiting method, and .NET restores the caller's execution context when an async method completes, so the write was discarded. The caller returned owning the semaphore while `IsHeldByCurrentContext` reported `false` — and any nested `Enter()` then blocked forever on a gate its own context held. Reachable remotely: `HandleIncomingMessageAsync` → error path → `ForceChannelFault`. The holder is now published from the caller's frame before the wait, and only its depth is raised in the continuation. **Queued writes lost their order** *(found in review)*. Sequence numbers are assigned under the gate, but independently queued work items reach the transport's FIFO send lock in arbitrary order, so a peer could reject a chunk with `BadSequenceNumberInvalid`. Writes are now appended to a chain: still off the caller's stack, but a write cannot start before its predecessor finishes. ### Four more this surfaced in CI, on paths only the integration suites reach Queuing the writes to fix the deadlock above turned out to have moved three implicit guarantees, and the gate had one more liveness bug. Each was reproduced locally against a clean baseline before it was changed. **A faulting channel's error message never reached the peer.** A listener channel sends a UASC `Error` describing why it faulted and closes the transport as its next statement. That worked only because the write started on the caller's stack, so the bytes reached the socket first; queued, the close discarded them and the client saw `BadConnectionClosed` instead of the certificate status it was told to expect. Nine `SecurityCertValidationTests` cases assert exactly that status. The terminal error is written inline again, on a path that touches the gate only to disclaim what it inherited and reports nothing — a channel that is already faulted is owed nothing beyond its buffer. **The certificate of a rejected sender leaked.** An asynchronous method cannot have `out` parameters, so the asymmetric read returns a record instead. That silently dropped an ownership guarantee: an `out` parameter reaches the caller even when the method goes on to throw, so the caller's `catch` disposed the sender certificate and reported it to the audit. A return value does not — nine leaked certificates, one per rejection case, which the leak detector fails the run on. The parsed certificate is now handed to the caller before anything that can reject the message, restoring both the disposal and the audit. **The gate could strand its own permit.** It released only when the holder's depth returned to zero, but that depth lives in an `AsyncLocal` and is therefore shared with every context forked from the owner's — a timer callback whose timer was created inside the region, for instance. Such a context entering the gate raised the shared depth, so the owner's own exit saw a non-zero depth and returned without releasing, and nothing afterwards could: the nested exit is not the owner. Every later entry on that channel blocked forever. `Opc.Ua.Features.Tests` hung after its tests had finished, with a receive loop waiting on the gate and the server unable to stop. The owner now releases when it leaves; depth still gates re-entrancy, but liveness no longer depends on a count another context can move. **Write completion throttled the writer.** Reporting a completed write sat inside the ordering chain, and the client channel implements that callback by entering the gate — so every write queued behind a gate the receive loop contends for on each incoming chunk. A session with fifty subscriptions publishing continuously had its publish requests throttled enough that the client kept topping them up past its own limit, which `Subscriptions.Classic` asserts against. Completion is now reported off the chain, which covers only what ordering actually requires. ### Still synchronous, and why Certificate, certificate request and revocation list signing cannot be made asynchronous at all: `X509SignatureGenerator.SignData` is invoked by .NET's own builders. Service faults and the `Reconnect` override are reached from synchronous call sites (`SendServiceFault` alone has eight) and are left as they are behind scoped, commented suppressions. All three are documented rather than left to be rediscovered. ## #4210 — PubSub This one produced a finding rather than the API the issue asked for, and the finding is the point: > **With a standard Security Key Service the key necessarily exists in process memory.** `GetSecurityKeys` (Part 14 §8.3.2) returns raw key bytes over the wire, so the "key never leaves the device" property #4192 achieved for client and server **cannot** be achieved for PubSub through the SKS pull profile. That is a property of the specification, not of this stack. A wrapped-key envelope would change what is on the wire and break interoperability with third-party key services and publishers, so it was rejected rather than built. What is achievable is delivered: the per-message AES-CTR and HMAC route through `ISymmetricCryptoProvider`, so a validated module performs them. `IPubSubSecurityPolicy` is **not** modified — new members would break every implementation of it; the provider is supplied through the policy constructor. `PlatformSymmetricCryptoProvider` gained AES counter mode, which the seam should have covered anyway since the algorithm enum names it. Key lifetime is also tightened: unpacking an SKS response copied key material into intermediates that `ByteString.Create` then copied again, leaving plain keys in the heap for the lifetime of the process. Those are now zeroed. ### Two wiring gaps this depended on, both found in review The registrations above were **inert**, so this section originally claimed more than the code did: - `CryptoProviderConfiguration` was applied nowhere outside tests, so `AddCryptoProvider(configure)` never reached the registry in a real host. The registry now applies every registered configuration when it is first resolved — which also affects #4207's DI story. - `PubSubSecurityWrapperResolver` selected `PubSubAes256CtrPolicy.Instance`, the provider-less static singleton, ignoring the provider-backed bundles registered for it. A `FipsOnly` deployment therefore certified while the platform did the work. The resolver now selects from the registered policies; the container and `PubSubApplicationBuilder` both supply theirs. A third, smaller one: `CryptoProviderBuilder.Use` published a **policy-scoped** random source as the process-wide nonce source, so `.For(purpose, policyUri).Use(hsm)` redirected nonce generation for every other policy too. Only an unscoped binding does that now. ## #4206 — a provider can contribute a security policy Added in review: the policy set was fixed at compile time, so a provider could contribute key custody but not a *profile*. It is now table-driven and open, and the blockers #4206 listed are gone. ```csharp using IDisposable registration = SecurityPolicies.Register( new SecurityPolicyInfo("urn:vendor:SecurityPolicy#CustomProfile", "CustomProfile") { PlatformSupport = () => true, SupportedCertificateTypes = [ObjectTypeIds.RsaSha256ApplicationCertificateType], IsDefault = true }); services.AddOpcUa() .AddCryptoProvider(crypto => crypto .For(CryptoPurpose.ChannelSymmetric, "urn:vendor:SecurityPolicy#CustomProfile").Use(module)) .AddSecurityPolicy(customPolicy); ``` - **The reflection is gone.** Four lookup tables were built by reflecting over the fields of `SecurityPolicies` and `SecurityPolicyInfo`. They are one explicit table now, which removes the **last reflection in `Security/Constants`** — so this is a trimming and Native AOT improvement as much as a feature. - **`SecurityPolicyInfo` is constructible from another assembly** and carries the metadata the helpers need, so `IsPlatformSupportedName`, `GetDefaultUris`, `MapSecurityPolicyToCertificateTypes` and `GetCurveFromCertificateTypeId` read that table instead of each being hand-written. Adding a policy is one entry rather than five edits. - **The dead `#if CURVE25519` conditional is deleted** from `Security/Constants`. - Registration is copy-on-write, so the read path stays lock-free. Shadowing a built-in policy requires `replaceExisting: true` and is reversible by disposing the registration. `RegisterLightsUpCurvePoliciesFromOutsideCore` is the acceptance test the issue asked for: it registers `ECC_curve25519` and `ECC_curve448` from the test project and asserts they become discoverable through the public API. **Registering a policy makes it advertised and resolvable; it does not supply the cryptography behind it.** For those two curve profiles the in-tree key agreement is still behind a compile symbol no project defines plus a BouncyCastle dependency, so a deployment lighting them up provides the operations through a provider. That division is the point: the policy set says *what* is offered, the provider says *who* performs it. ## Removing the gate's re-entrancy Folded in from #4242, which was merged into this branch. `ChannelGate` was re-entrant, and that forced the design behind most of the defects above. A holder that awaits releases its thread, so ownership could not be tracked by thread identity — it had to live in an `AsyncLocal<Holder?>`. A logical context is *inherited*, so anything started while the gate was held inherited the right to re-enter and ran inside the guarded region alongside whatever started it, unless it opted out by hand via `LeaveInheritedContext()` at each of eight sites, enforced by review alone. Five of the defects listed above trace to that mechanism, including both CI hangs. So the *requirement* was removed rather than the mechanism, and the `AsyncLocal` then deleted itself. Every path that re-entered now calls a lock-free `Core` method (`ForceChannelFaultCore`, `ShutdownCore`, `ForceReconnectCore`, `CompleteConnectCore`), and three regions stopped running foreign code under the lock: the `RequestReceived` dispatch is deferred until the gate is released, `OnTransportError` no longer wraps the `HandleSocketError` virtual, and the callback setters and `EndpointDescription` publish a single reference with a volatile write instead of locking — the last is load-bearing, because `AsyncResultBase.OperationCompleted` invokes the reverse-hello callback inline while the gate is held. `ChannelGate` is now a `SemaphoreSlim(1,1)` behind a `Releaser`; `Holder`, the thread-identity fallback, `LeaveInheritedContext`, `IsHeldByCurrentContext` and all eight disclaimer calls are gone, with the call shape at all 32 sites unchanged. `SaveIntermediateChunk`, `GetSavedChunks` and `DoMessageLimitsExceeded` gained a `gateHeld` argument, because the client reaches them both with and without the gate and must pick the matching teardown — without it an oversized `OpenSecureChannel` response deadlocked the receive loop against itself. The remaining paths were not found by reading the code, since that is what missed them originally. A temporary strict mode made every re-entrant acquisition throw with its stack, the suite was run under it until clean, and it was deleted with the re-entrancy it measured. It caught two sites review had missed: the `EndpointDescription` **property getter** taking the gate, and the `RequestReceived` dispatch to application code happening inside the guarded region. Two deliberate residuals are documented for reviewers: `Dispose` no longer takes the gate (unavoidable — it is reached from `ChannelFaulted` with the gate held, where the old re-entrant acquisition excluded nothing anyway), and the deferred request dispatch builds `SecureChannelContext` just after gate release rather than under it. ## Testing New: the three seams and their platform implementations, facet discovery and resolution precedence, `FipsOnly` refusing a provider that cannot serve what it was bound to, AES counter mode (including a non-block-aligned round trip and a check that the counter carries, since one that did not would silently reuse key stream), `ChannelGate` (exclusion, re-entrancy across an await, re-entrancy on a reused thread, and the deadlocks above), the asynchronous channel paths, and the PubSub policy provider. Each review finding got a test that fails without its fix: - `ContendedEntryStillRecordsTheHolderAsync` — established contention deterministically, then re-entered; failed on the pre-fix gate. Removed with the re-entrancy it covered; `ChannelGateTests` now asserts the non-re-entrant contract instead. - `SecureClientAndTcpListenerKeepChunkOrderUnderConcurrentRequestsAsync` — 60 concurrent multi-chunk `SignAndEncrypt` requests over a real loopback listener; failed 3 runs out of 3 with the previous dispatch, passes 3 out of 3 with the chain. - `DependencyInjectionRoutesPubSubCryptoThroughTheRegisteredProviderAsync` — builds a container, registers a counting provider, wraps a message and asserts the provider actually performed the encryption and the signature. This is the assertion the PubSub claim above now rests on. - `PolicyScopedRandomBindingDoesNotEscapeItsPolicy`. Two further assertions are the ones that would catch a silent defect: a provider and the platform must produce **byte-identical** output — a validated module that disagreed could not interoperate — and with a software key the returned `ValueTask` must report `IsCompletedSuccessfully`, which is what makes "ordering is unchanged" checkable rather than merely asserted. `SymmetricChannelCryptoBenchmarks` gains a provider-registered variant beside the existing baseline, so the cost of the indirection is measured rather than assumed. Verified locally, every suite at or above its recorded baseline: `dotnet build UA.slnx -c Release` **0 warnings, 0 errors on all six target frameworks**; Core **4413** (net10.0); Core.Security **617**, with its certificate-leak detector clean; PubSub **1350** plus PubSub.Diagnostics **54** and PubSub.Udp **233**; Features **156**; Subscriptions.Classic **32**; Server integration **4053**; Client integration **2123**; MigrationAnalyzer **151** and MigrationAnalyzer.Core **13**. Zero failures in each. Core moved from 4388 to 4381 when the gate change landed — eight re-entrancy tests were removed along with the behaviour they covered, and one deadlock regression test added — and has since grown to 4413 with the registry isolation, provider-output and security-review tests. `ChannelGateTests` was rewritten around the new contract — exclusion across both entry modes, an uncontended asynchronous entry not suspending, re-entry blocking by contract, and the property that used to be the hazard: work started while the gate is held is excluded without having to opt out. Every CI failure this PR produced was reproduced locally, diagnosed against a clean `origin/master` baseline in a second worktree, and fixed at the source. Two hangs were found from `--blame-hang-timeout` dumps read with `dotnet-dump`. One failure that looked like a candidate was confirmed **not** to be: an intermittent `Opc.Ua.Aot.Tests` connect failure, pre-existing on `origin/master` (5 of 8 baseline runs failed, versus 3 of 14 here). ### The publish request overshoot `Subscriptions.Classic.PublishRequestCountAsync` failed on the Windows net10.0 leg with **207** outstanding publish requests against a cap of `max(maxServerPublishRequest, subscriptions)` = 50. It failed in 2 of 6 pipeline builds on this branch, never on `master` in 13 sampled builds, and never locally in 12 attempts including under deliberate CPU contention. The mechanism is a genuine race. `ClassicSubscriptionEngine.QueueBeginPublish` was a **check-then-act**: it read `GoodPublishRequestCount`, compared it against `GetDesiredPublishRequestCount(false)`, and issued a publish if it was below. It runs on every publish completion, so *n* completions that overlap each read the same value, each conclude they are below the cap, and each issue — overshooting by up to *n* per round. The read also *lags*: a request only becomes visible to `GoodPublishRequestCount` once `AsyncRequestStarted` has recorded it, which happens after the request has already been sent. That code is pre-existing and was untouched by this PR. What this PR changed is how hard the race is hit: the server used to hold the channel gate across request processing, which serialised every request on a channel, so publish completions arrived one at a time. Dispatching `RequestReceived` outside the gate — an intended improvement, since a channel should not serialise on application request-processing time — lets them complete concurrently. Rather than change the publish pipeline blind, the race was first made reproducible. `ConcurrentPublishReEvaluationDoesNotExceedDesiredRequestCount` drives 64 barrier-released completions against a mocked context whose count lags the send exactly the way the session's does. It issued **64 requests where 5 were desired** — the same defect the CI leg hit, now deterministic and running in milliseconds. The engine now tracks the requests it has in flight and reserves a slot with a compare-exchange before sending, so the decision and the send are one atomic step. The reservation is released when the request completes, when the send is declined, or when it throws. The repro test then issues at most the desired count. `StartPublishing` is deliberately left **uncapped**. It is the recovery valve that refills a pipeline whose requests are outstanding but no longer expected to return, so gating it on the in-flight count would remove the only path out of a stalled pipeline. An earlier revision of this fix did gate it, and that is the one change here that could not be justified on the evidence — it was reverted. The assertion was **not** loosened, retried away, or marked flaky: it caught a real throttling regression earlier in this PR and is worth keeping sharp. Validated: the repro test red before the fix and green after; `Subscriptions.Classic` 32/32 **five** consecutive runs on net10.0 and once on net48; `Opc.Ua.Client.Tests` 2124 on net10.0 across four consecutive runs and 2127 on net48; `Opc.Ua.Subscriptions.Tests` 604 passed with this fix in place. ### The SSH.NET advisory that turned CI red repo-wide Partway through this work every build job began failing with `NU1903: Package 'SSH.NET' 2025.1.0 has a known high severity vulnerability` ([GHSA-q939-rpr3-3284](https://github.com/advisories/GHSA-q939-rpr3-3284), `<= 2025.1.0` affected, first patched `2026.0.0`). Nothing in the dependency graph changed — the advisory was published. Build 17026 failed on it where build 17000, two hours earlier on an identical graph, had passed every build job. SSH.NET is not referenced anywhere in the repository; it arrives transitively through `Testcontainers.Kafka`. `master` landed the remediation independently while this was being investigated (a central transitive pin to `2026.0.0`, which `CentralPackageTransitivePinningEnabled` exists for). This branch briefly carried its own identical pin, which made the merge commit hold **two** `PackageVersion` entries for SSH.NET and fail restore with `NU1506`. The duplicate is removed and `master` is merged, so the pin now comes from one place only. The audit was **not** suppressed: `NuGetAudit` stays enabled and `NU1903` stays an error. Verified after the merge — `dotnet restore UA.slnx` reports neither NU1903 nor NU1506, and `dotnet build UA.slnx -c Release` is **0 warnings, 0 errors** with the audit live. The remaining red check in build 17026 was not from this change either. `aot-ubuntu-latest` is the intermittent `Opc.Ua.Aot.Tests` failure already noted above — `ReconnectSessionAsync` timed out at 30s with 135 of 136 passing. It remains below the `origin/master` baseline rate (4 of 15 sampled here against 5 of 8 on the baseline), and the managed reconnect paths are green throughout: `Opc.Ua.Client.Tests` 2124 across four consecutive runs and `Opc.Ua.Subscriptions.Tests` 604. ## Documentation `docs/CryptoProvider.md` gains sections on substituting the symmetric primitives, using a key served over a network, and PubSub — including what cannot be claimed and why, the scope rule for a random-source binding, and how to supply a provider-backed policy outside the container. `docs/MigrationGuide.md` covers the `[Obsolete]` channel virtuals and `DataLock`, and the non-re-entrant gate's contract for channel subclasses. `docs/WhatsNewIn2.0.md` is updated. `plans/cryptooffboard.md` is deleted now that the work it planned has landed. ## Note for reviewers This touches the secure channel handshake, which is the most security-critical code in the stack. Nine distinct concurrency and lifetime defects were found and fixed in it during this work — three during development, two in review, four from CI — so `ChannelGate`, the write path and the certificate ownership across the new suspension points deserve the closest reading. Five of those nine traced to the gate's `AsyncLocal`, which is why the re-entrancy was subsequently removed and the `AsyncLocal` deleted. The wrapper/`Core` split and the two documented residuals in that section are the parts most worth a second reading. The #4206 work removes the last reflection from `Security/Constants` and rewrites the policy lookup tables, so `SecurityConfiguration.SupportedSecurityPolicies` and the endpoint set it feeds are worth a second look: an earlier revision of it changed that set and broke six server tests, which is now covered by `SecurityConfigurationBuildsExpectedSupportedPolicySet`. ## Related Issues - Fixes #4206 - Fixes #4207 - Fixes #4208 - Fixes #4210 - Follow-up #4250 — this PR makes the security policy set injectable, but nothing in the stack resolves `ISecurityPolicyRegistry` from DI yet, so every production path still lands on the `Default` fallback. #4250 tracks threading it through `Session` and `UaSCBinaryChannel`, which is what makes the injection points already added here load-bearing. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Copilot-Session: 7a68ca72-83e8-496c-b2f5-3df4c718320b Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b	2026-08-15 05:23:20 -07:00
romanett	d547747e22	Generate ReferenceNodeManager address space from a NodeSet2 model (#4220) ... # Description The CTT/compliance `ReferenceNodeManager` built its ~6,000 node address space entirely in imperative C#. This PR migrates it to a source-generated `[NodeManager]` partial driven by an exported NodeSet2 model (`ReferenceServer.NodeSet2.xml`), mirroring the `MinimalBoilerServer` sample and completing the goal of issue #4204. The address space is now materialized from the model by the source generator. The large orchestration body was kept but converted from create-based to **find-instead-of-create**: each `Create*` primitive now looks up the model-materialized node via `FindPredefinedNode<T>` and re-applies runtime values, DataAccess metadata, and write handlers, rather than constructing nodes from scratch. This keeps the deterministic value seeding and behavior wiring intact while the structure comes from the model. ## Notable details for reviewers - **Idempotent reference wiring.** The mesh references in the `References` folder are already present in the model, so the historic manual `AddReference` calls would throw on duplicates. Added a small `AddReferenceIfMissing` helper so those calls are safe. - **Non-string NodeIds.** The `NodeIds` folder assigns numeric / GUID / opaque NodeIds. The model exports those nodes with their final ids, so find-by-string-path no longer works; added `CreateVariableById(NodeId)` to locate them by their real id. - **Generator bug fix (worth a careful look).** `ModelDesignExtensions.GetValueRankAsCode` reconstructed `ValueRank` as `dimensions.Length + 1`, so a node with `ArrayDimensions="3,3"` was emitted as `ValueRank = 3`. This affected **any** NodeSet2 with a fixed multi-dimensional array (rank-3 matrices, `BadTypeMismatch` on whole-matrix writes, wrong decoded dimensions). Corrected to `dimensions.Length` and updated the unit tests that had codified the off-by-one. ## Validation - `ReferenceServerTest`: 44/44 on net10.0 and net48 - `Opc.Ua.SourceGeneration.Core.Tests`: pass on net8.0/net9.0/net10.0/net48 - Integration on net10.0: ComplexTypes, Client, History, Sessions, Subscriptions all pass - Clean build, no new analyzer warnings ## Related Issues - Fixes #4204 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [ ] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-08-15 05:22:31 -07:00
Copilot	aad9e9b1c1	Document Opc.Ua.Core.Diagnostics trim/AOT exclusion and suppress reflection IL2075 warnings (#4249) ... ## Description `Opc.Ua.Core.Diagnostics` has two trim/AOT defects that are invisible today because the assembly is not marked `IsAotCompatible`: `NicCaptureSource.StartAsync` carries `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` that its interface member `ICaptureSource.StartAsync` lacks (IL2046/IL3051), and `ServiceCallReassembler` reflects over `IEncodeable` properties by name (IL2075 ×3), which under trimming silently returns `null` instead of failing loudly. ### NicCaptureSource / ICaptureSource mismatch - Left as-is: `ICaptureSource` stays unannotated since most capture sources don't need the SharpPcap dynamic-loading requirement, and `NicCaptureSource` genuinely does. - Documented explicitly in `Opc.Ua.Core.Diagnostics.csproj` that the project is deliberately **not** `IsAotCompatible`/`IsTrimmable` for this reason, mirroring the existing pattern in `Opc.Ua.Mcp.Diagnostics.csproj`. ### ServiceCallReassembler reflection - Verified the reflection-based property lookup is intentional (avoids allocating via `ToArray()` just to get a count; covered by existing tests). - Added `[UnconditionalSuppressMessage]` with justification and a TODO to `GetCollectionCount`/`GetPropertyString`, pointing at the source-generated-accessor approach suggested as the long-term fix, instead of leaving the reflection silently unannotated. ```csharp // TODO: Replace these string-keyed property lookups with a source-generated // accessor ... until then, GetType().GetProperty(...) here is unavoidable // and is suppressed rather than silently left unannotated. [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern", Justification = "...")] private static int? GetCollectionCount(IEncodeable message, string propertyName) ``` Verified by temporarily enabling `IsAotCompatible` locally: all 3 IL2075s resolve, leaving only the accepted/documented `NicCaptureSource` IL2046/IL3051 mismatch. ## Related Issues ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [ ] I have added all necessary documentation. - [ ] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #4236 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-08-14 17:41:28 +00:00
romanett	6e0ed89d3f	Make UANodeSetSerializers.g.cs generation deterministic (#4252) ... ## Problem Regenerating `src/Opc.Ua.Types/Schema/UANodeSetSerializers.g.cs` via `UANodeSetSerializers.ps1` produced ~144 lines of no-op churn on every run. The sgen tool (Microsoft.XmlSerializer.Generator) emits the Reader `idNN_Name` field declarations and the matching `InitIDs()` initializers in a hash-seed-dependent order: the assigned id numbers are stable, but their emission order shuffles between runs. This made an otherwise no-op regeneration produce a large, noisy diff that was painful to review. ## Fix - Added a canonicalization pass in `UANodeSetSerializers.ps1` that sorts each of the two affected contiguous blocks (the `string idNN_Name;` field declarations and the `idNN_Name = Reader.NameTable.Add(...)` initializers) by numeric id ascending, so re-running on unchanged input yields byte-identical output. - The name regex was widened to `@"[^"]*"` so entries with an empty name (e.g. `id3_Item`) or URI-style names do not split the block into independently-sorted runs (which would keep it unstable). - Regenerated the committed `.g.cs` once through the improved script; the resulting diff is sort-only. ## Also fixed: tool download Replaced `Expand-Archive` (which rejects the `.nupkg` extension on Windows PowerShell) with `System.IO.Compression.ZipFile.ExtractToDirectory`, so a clean run with an empty tool cache works end-to-end on both Windows PowerShell 5.1 and PowerShell 7+. ## Validation - From an empty tool cache, the full download + extract + generate path succeeds. - A second run is a byte-identical no-op (`git diff --exit-code` = 0). - `dotnet build src\Opc.Ua.Types\Opc.Ua.Types.csproj -c Release -f net10.0` builds with 0 warnings and 0 errors. - All AOT post-processing is preserved: custom header, `#if NET5_0_OR_GREATER` wrapper, module IL2026/IL3050 suppressions, `[RequiresUnreferencedCode]`/`[RequiresDynamicCode]` on the InitCallbacks() overrides (Writer = RequiresUnreferencedCode only, Reader = both), pragmas, UTF-8 BOM, and CRLF line endings. No hand-edits to `.g.cs`; the fix lives entirely in the script so future regenerations stay stable. ## Also: Load UserAccessLevel from NodeSet2 model Bundled here because it regenerates `UANodeSetSerializers.g.cs`, which this PR owns. - `UANodeSet.cs`: adds the `{Member}Specified` XmlSerializer convention to `UAVariable.UserAccessLevel` (new `UserAccessLevelSpecified` companion; removed the `[DefaultValue((uint)1)]`) so an explicitly-present `UserAccessLevel` attribute is distinguishable from the schema default of 1. - `UANodeSetSerializers.g.cs`: regenerated - the writer now emits `UserAccessLevel` when `UserAccessLevelSpecified`, and the reader sets `UserAccessLevelSpecified = true`. Regenerating produced only these two behavioral changes plus removal of pre-existing empty `if (o.@MemberSpecified) { }` no-op blocks (see below); no `idNN` re-sort, so determinism held. - `UANodeSetHelpers.cs`: the runtime importer honors an explicitly-specified `UserAccessLevel` instead of always deriving it from `AccessLevel`. - Source generator (`ModelDesign.cs` `RawUserAccessLevel`, `ModelDesignExtensions.GetUserAccessLevelAsCode`, `NodeSetToModelDesign.cs`): emit the verbatim `UserAccessLevel` when the NodeSet sets it, otherwise derive from `AccessLevel` (matching the runtime importer). ### Generator normalization for `{Member}Specified` sgen emits an extra empty `if (o.@MemberSpecified) { }` no-op block near `WriteEndElement` for every member that uses the `{Member}Specified` convention. Adding the companion to `UserAccessLevel` would have introduced one such block, and the committed file already contained several for other members (`LastModified`, `AccessRestrictions`, `PublicationDate`). `UANodeSetSerializers.ps1` now strips these empty guard blocks during post-processing, so the generated file stays free of dead code and a re-run of the generator remains a byte-identical no-op. The real, non-empty guard around the `WriteAttribute` call is preserved. The ReferenceServer sample application of this feature is intentionally excluded and lands with #4220. ### Validation (UserAccessLevel) - `dotnet build src\Opc.Ua.Types\Opc.Ua.Types.csproj -c Release -f net10.0` and `dotnet build tools\Opc.Ua.SourceGeneration.Core\Opc.Ua.SourceGeneration.Core.csproj -c Release`: both 0 warnings / 0 errors. - `dotnet test tests\Opc.Ua.SourceGeneration.Core.Tests -c Release -f net8.0 --filter "FullyQualifiedName~NodeSetToModelDesign|FullyQualifiedName~UserAccessLevel"`: 16 passed, 0 failed. - Re-running the generator remains a byte-identical no-op. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>	2026-08-14 08:00:13 -07:00
romanett	34fe1a1567	Emit WriteMask from the model in the source generator (#4253) ... ## Summary Isolates a self-contained OPC UA source-generator bug fix, extracted from PR #4220 (which targets a different base branch) so it can merge independently against `master`. ## Fix: Emit node WriteMask from the model **Bug:** The `NodeState` source generator hardcoded `global::Opc.Ua.AttributeWriteMask.None` for both the `WriteMask` and `UserWriteMask` attribute replacements. As a result, attributes declared writable in a NodeSet2 model (for example `WriteMask=96`) were emitted as `None`, so writes to those attributes failed with `BadNotWritable`. **Change:** - `tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs` - the common node replacements now emit `root.GetWriteMaskAsCode()` for the `WriteMask` and `UserWriteMask` tokens instead of the hardcoded `AttributeWriteMask.None`. - `tools/Opc.Ua.SourceGeneration.Core/Schema/ModelDesignExtensions.cs` - adds a `GetWriteMaskAsCode(this NodeDesign node)` helper that returns `global::Opc.Ua.AttributeWriteMask.None` when `WriteAccess == 0`, otherwise `(global::Opc.Ua.AttributeWriteMask){writeMask}`. ## Tests - `tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/ModelDesignExtensionsTests.cs` - adds `[TestCase]`-driven coverage for `GetWriteMaskAsCode` across representative write-mask values plus a null-node test. ## Validation - `dotnet build tools\Opc.Ua.SourceGeneration.Core\Opc.Ua.SourceGeneration.Core.csproj -c Release` - 0 warnings, 0 errors. - `dotnet test tests\Opc.Ua.SourceGeneration.Core.Tests\Opc.Ua.SourceGeneration.Core.Tests.csproj -f net8.0 -c Release` - 3775 passed, 0 failed, 8 skipped. ## Note on the Views fix (excluded) A second generator fix (preserve forward same-namespace `Organizes` references to `ViewDesign` targets in `NodeSetToModelDesign`) was intended to ship here as well, but it was excluded. That fix compensates for behavior introduced by #4220 where imported Views are promoted to top-level `ViewDesign` nodes and their `ParentNodeId` is cleared. On `master` that promotion does not happen (Views remain children of their folder), so the fix and its test do not apply and cannot be cleanly isolated. It will land with #4220. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-08-14 07:57:44 -07:00
romanett	cc4791e2fd	Import NodeSet2 Views as standalone top-level nodes in the source generator (#4259) ... ## Problem The NodeState source generator mishandled `UAView` nodes imported from a NodeSet2 model. When an exporter set a View's `ParentNodeId` to an organizing folder, the importer/generator absorbed the View into that folder's `Children` and tried to emit an invalid `AddChild(...)` call. Because `ViewState` derives from `NodeState` (not `BaseInstanceState`), a View can never be an `AddChild` component, so the generated code was invalid. ## Fix Make Views standalone top-level predefined nodes linked purely via `Organizes` references — mirroring how `DataType`/`ReferenceType` type nodes and `DataTypeEncoding` are handled: - **`tools/Opc.Ua.SourceGeneration.Core/Schema/NodeSetToModelDesign.cs`** — clear the `UAView` `ParentNodeId` so the View stays a standalone top-level node, keep the organizing folder's forward `Organizes` references to those Views, and carry the View's `ContainsNoLoops` attribute through to the `ViewDesign`. - **`tools/Opc.Ua.SourceGeneration.Core/Generators/NodeStateGenerator.cs`** — return `null` for `ViewDesign` in the `AddChild` and `ReplaceChild` template paths so Views are never emitted as components. - **`tests/Opc.Ua.SourceGeneration.Core.Tests/Schema/NodeSetToModelDesignTests.cs`** — new `ViewImport` NodeSet constant and `ImportPreservesFolderOrganizesReferencesToViews` test verifying Views are imported as top-level nodes while the organizing folder retains its forward `Organizes` references to them. ## Validation - `dotnet build tools\Opc.Ua.SourceGeneration.Core` (Release): 0 warnings / 0 errors. - `dotnet test tests\Opc.Ua.SourceGeneration.Core.Tests` (net8.0, filtered to `NodeSetToModelDesign`): all 17 tests pass, including the new `ImportPreservesFolderOrganizesReferencesToViews`. Extracted from the larger PR #4220 work — this PR contains only the source-generator View fix (no WriteMask, UserAccessLevel, NodeSet2.xml, or ReferenceNodeManager changes). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-08-14 07:57:06 -07:00
romanett	2c2bfa7080	Fix off-by-one ValueRank in the NodeState source generator (#4258) ... ## Summary `ModelDesignExtensions.GetValueRankAsCode` in the NodeState source generator computed the ValueRank for a fixed-size multi-dimensional `ArrayDimensions` string (e.g. `"3,3"`) as `dimensions.Length + 1`, which over-counted the rank by one. The number of comma-separated entries in `ArrayDimensions` already **is** the ValueRank — `"3,3"` is a two-dimensional array with ValueRank `2`, not `3`. ## Changes - `dims = dimensions.Length + 1` → `dims = dimensions.Length` (entry count already equals the ValueRank). - Added a guard: when there are zero entries (empty or all-commas `ArrayDimensions`, which `Split(..., RemoveEmptyEntries)` reduces to length 0), the method now returns `OneOrMoreDimensions`. - Refreshed the stale explanatory comment / removed the obsolete `TODO`. - Updated the `GetValueRankString_*` unit tests in `ModelDesignExtensionsTests.cs` to the corrected expected ranks. ## Validation - `dotnet build` of `Opc.Ua.SourceGeneration.Core` (Release): 0 warnings / 0 errors. - Filtered `GetValueRankString` tests on net8.0: 18/18 passed. Extracted from the larger PR #4220 work; this PR contains **only** the ValueRank fix (two files). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>	2026-08-14 07:56:23 -07:00
Marc Schier	80db326567	Fix MCP diagnostics tool contracts and capture failures (#4241) ... # Description Fix the MCP packet-diagnostics tool contract and the capture failures reported in #4238. - Accept and emit canonical hyphenated capture source and format names while preserving CLR-name and integer compatibility. - Restore typed enum constraints in generated MCP schemas and surface actionable diagnostics errors to MCP clients. - Stop `list_interfaces` from reading `LinkType` on unopened SharpPcap devices. - Close failed SharpPcap handles before retrying NIC opens and report permission failures immediately. - Normalize capture roots, remove duplicated path segments, and use a lazy secure per-user or atomic temporary fallback. - Align MCP/diagnostics documentation and interop-agent guidance. ## Related Issues - Fixes #4238 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Copilot-Session: 641a4356-aa8d-45e6-9077-eeb50b9b7e0a	2026-08-12 10:33:56 +02:00
Marc Schier	1cca744b5c	Retire the last exposed locks: node, browser, server diagnostics and application properties (#4222) ... Closes #4201. Closes #4202. Closes #4203. Closes #4215. Closes #4216. (#4215 and #4216 are @marcschier's originals from the #4188 review; #4202 and #4201 are my later duplicates of the same two findings. All four describe the two changes in section 1 and are closed together.) **This PR now carries what was #4221 as well.** The two were a stack; they have been unstacked and combined here, and #4221 is closed. Everything below the "Read the server diagnostics" section is the work that was previously reviewed as #4221. --- ## 1. The browse path guards itself, and the last node locks are gone `AsyncCustomNodeManager` kept two `lock (source)` sites around `CreateBrowser`, carrying a NOTE that browser creation was not internally synchronized. `NodeBrowser` still handed its lock to derived types through `protected Lock DataLock`. `NodeState.CreateBrowser` now holds a browse lock across `PopulateBrowser` and `OnPopulateBrowser`, so browser construction on a node is serialized — which is what the external `lock (source)` was actually providing, and what a handler that mutates the node during population (the historian's lazy config installer) depends on. Both `lock (source)` statements and the NOTE are gone. **What that lock does not do**, stated plainly in the XML docs and in `docs/NodeManagers.md` rather than over-promised: it is not an atomic snapshot across the node's children, notifiers and references. Writers take those collections' own locks, not the browse lock. The browser is a point-in-time copy and each collection is read consistently, but the combination is not a transaction. No `lock` takes a `NodeState` as its sync root any more. That was the last thing keeping `NodeState` from being documentably self-guarding, so the other eleven went with it: `lock (activeNode)`, `lock (target)`, `lock (source.Node)`, `lock (target.Node)`, `lock (notifier)`, `lock (handle.Node)` in `AsyncCustomNodeManager`, and `lock (node)` in `CustomNodeManager2`. Most were a check-then-act pair now served by `NodeState.AddReferenceIfMissing`, which does the check and the insert under the node's own lock. `NodeBrowser.DataLock` is removed. A browser is **single-consumer**: it belongs to whoever created it, and both server browse paths already serialize on the owning side. The exposed lock only added an inheritance-level locking contract that a derived browser could not reason about. ## 2. Read the server diagnostics under the lock every writer takes `OnUpdateDiagnostics` locked the diagnostics **payload** rather than the lock every writer takes through `UpdateServerDiagnostics`, so the snapshot a client reads from the `ServerDiagnostics` node was never excluded from a concurrent writer. Both halves matter, and the second is the one that is easy to miss: ```csharp lock (m_diagnosticsLock) // was: lock (ServerDiagnostics) { value = Variant.FromStructure(ServerDiagnostics, copy: true); // was: copy defaulted to false } ``` `copy: true` is what makes it a snapshot. `FromStructure(value, copy: false)` does `new ExtensionObject(value, copy)` — it wraps the **live** structure without copying it, so the caller would read the fields long after the lock was released and see them change underneath. Swapping the lock alone would have looked like a fix while still excluding nothing. ## 3. Analyzer numbering The browser rule is **UA0027**, not UA0025. This branch had independently taken UA0025 for `NodeBrowser.DataLock` while #4188 published UA0025 for `ILocalNode.DataLock` and UA0026 for `BaseVariableValue.Lock`. Master's are shipped, so they keep the numbers: | | UA0025 | UA0026 | UA0027 | |---|---|---|---| | master (#4188) | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | — | | **this PR** | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | **`NodeBrowser.DataLock`** | Both sides had written a different `UA0025Tests.cs`, which arrived as an add/add conflict. They are now split: `UA0025Tests.cs` holds master's `ILocalNode` tests, `UA0027Tests.cs` holds the `NodeBrowser` ones. UA0024's help text also drops its reference to `ReadServerDiagnostics`, which #4193 removed as unused, and names `ISession.ReadDiagnostics` / `ISubscription.ReadDiagnostics` instead — those do still exist. ## 4. Two more locks reachable from outside their owner **`ApplicationConfiguration.PropertiesLock` is removed.** It returned the properties dictionary itself, so `lock (configuration.PropertiesLock)` was `lock (configuration.Properties)` — the lock *was* the data it guarded, the same shape as #4203. It had exactly one production consumer (`OAuth2CredentialCollection.Load`, a get-or-add with `ParseExtension` called inside the lock), so it could not simply be deleted. `Properties` is now a `ConcurrentDictionary` — same public type, every operation atomic on its own — and the one multi-operation need gets a member of its own: ```csharp public T GetOrAddProperty<T>(string key, Func<T> valueFactory); ``` It deliberately does **not** invoke the factory under a lock, so `ParseExtension` now runs outside any critical section. Holding a lock across a caller-supplied callback is exactly the shape that produced the attribute/diagnostics deadlock earlier in this work. The two existing tests asserted only `PropertiesLock is not null`, so they tested nothing but its presence. They are replaced by tests of the contract: factory runs once for a missing key, not at all for a present one, 32 concurrent callers all observe the same instance, null arguments throw. `PropertiesLock` ships in 1.5.378, so this is a breaking removal — migration guide section plus analyzer **UA0028**. **`SharedDefinitionCache` no longer hands out its lock.** Three sites in `SharedKeyValueSubscriptionStore` took the nested cache's `Lock` to operate on the cache's own dictionary. Both are private now, behind `ReplaceAll` and `CloneWhere`. `SnapshotCommitLock` stays exposed on purpose: it is a `SemaphoreSlim` sequencing an async commit across `await` points, which `System.Threading.Lock` cannot span, so folding it in would change behaviour rather than hide a lock. ## Verification Merged `master` (`0edfe237a`, including #4234) with no conflicts. - `dotnet build UA.slnx` — 0 warnings, 0 errors - `Opc.Ua.Server.Tests` — 4074 passed - `Opc.Ua.Types.Tests` — 8501 passed - `Opc.Ua.MigrationAnalyzer.Tests` — 151 passed on all five TFMs (138 → 151, the new ones being UA0027 and UA0028) - `Opc.Ua.Core.Tests` — 4249 passed - `Opc.Ua.Redundancy.Server.Tests` — 573 passed ## Note for the reviewer @romanett — your approval was given when this PR's diff was the 7 diagnostics files, because it was measured against `marcschier/node-browse-locks`. Now that the two are combined and the base is `master`, the diff is 23 files, and the extra 16 are the node/browser lock work that was #4221. GitHub kept the approval across the retarget, but it did not cover that code, so please re-review rather than let it ride. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63 Copilot-Session: 0ee22c1d-c319-4257-bf78-a5a077a1824a	2026-08-11 22:14:10 +02:00
Marc Schier	466c5cb982	Robot intent follow-ups: multi-controller executors, spec fixes, and an MCP tool library (#4195) ... Follow-ups to #4165, plus the work that followed from re-checking this branch against the specification. Everything started as a defect found by review or by probing the running code; the specification is unchanged except where noted. The TLS certificate lifetime fix (#4196) and the OpenUSD work (#4198) were both merged into this branch. #4196 has since landed on master, so it no longer appears in this diff. #4198 was closed in favour of this PR and its content is here. The MCP trim/AOT work (#4228), the `DataGenerator` null `NodeId` fix (#4227) and the NodeSet external-parent import fix (#4229) have since been merged in as well. All three are closed in favour of this PR and their content is here, described under *Making the MCP libraries trim and AOT clean*, *A generator that returned the value you opted out of* and *A NodeSet parent that lived outside the import batch* below. ## A two-arm cell could not be built `IntentExecution` carried no controller identity, and there was effectively one `IIntentExecutor` per server. An executor handed an execution therefore could not tell **which arm** it was for, so a cell with two `IntentControllerType` instances — an ordinary configuration — was not expressible through the builder or DI. Both halves are addressed: - `IntentExecution` now carries `ControllerId` (`NodeId`, the OPC UA identity) and `ControllerName`, so one shared executor can discriminate; - DI gained per-controller registration — `AddRobotIntentExecutor<TExecutor>(controllerBrowseName)` and an instance overload — so two genuinely different executors can coexist. Resolution order is `WithExecutor` → per-controller DI registration → the single DI executor → a build-time throw. There is still **no silent fallback** to a rejecting executor; that was a Critical defect fixed in #4165 and it stays fixed. `IIntentExecutor` is unchanged, so this is purely additive. ## Declarations that were quietly untrue - **`Accepts<T>` silently discarded a redeclaration.** Declaring an intent type twice — say to narrow `cancelSupported` on the second call — kept the first, wider declaration and said nothing. That is the same class of defect as the capability-honesty problems #4165 fixed (`PauseSupported` declaring a pause the host could not perform; `Aborting` degrading silently to `Buffered`). Identical redeclarations remain idempotent; conflicting ones now throw `BadInvalidArgument`. - **`RI-Interop-40010` assumed Annex B item 3 rather than checking it.** It now requires exact set equality between the OPC 40010 `TaskProgramName` values and the Robot Intent `ProgramType.ProgramId` values. Both sides are readable from the address space without commanding the robot, which is what makes the requirement structural under §12.1. ## Two things the specification requires that this did not do Re-checking the implementation against the merged draft revision turned up two genuine violations. Both are fixed, and the NodeSet is untouched by either. - **Every graphed mission reported `Failed` on success.** `MissionRules.SelectTransition` returned `null` for two different situations — a graph *terminus*, which is the normal success path, and a branch point whose conditions all evaluated false — and the caller collapsed both into `Failed`/`NoTransition`. A three-way result (`Terminus` / `Selected` / `NoConditionMatched`) separates them, so a mission that runs to its end now completes. The failure reason surfaces through the inherited Part 10 result container rather than a new member. - **Facet URIs were never published.** An upstream `may` became a `shall` in the revision this branch adopts. `RobotIntentConformanceUris.Facets` held 31 URIs with **zero** production references — the set existed but nothing emitted it. The published set is now derived from `RobotIntentFacetCalculator`, the same authority that already computed `SupportedFacets`, rather than from a second hand-maintained list that could drift; a reflection test pins the two together. ## An easy client mistake `RequestAuthorityAsync` returns a lease the caller must remember to inspect. Forgetting makes every later submission fail with `ControlNotOwned` for reasons that are invisible at the call site. `RequireAuthorityAsync` throws instead, and names the current owner. `RequestAuthorityAsync` is untouched — inspecting and branching is legitimate. ## Letting an agent drive the robot New `tools/Opc.Ua.Mcp.Robotics` gives a language model the intent API as MCP tools: 36 across discovery, monitoring, direct control and missions, behind a `Robotics` tool profile, composable into any MCP host exactly as the existing OPC UA tool packages are. The `IntentViewerClient` sample hosts them behind `--mcp` (stdio by default, HTTP when `--view`, since MCP stdio and the in-process viewer would otherwise share stdout), and the sample gained a pallet scene so an agent can pick and stack parts and watch the result in the OpenUSD viewer. All the scene logic is server-side — no viewer or connector changes. Four gaps in the client API had to be filled first, since the tools are a thin layer over it and could not have been written honestly without them: `ReadStateAsync`, `ListOperationsAsync`/`ListMissionsAsync`, controller-level `CancelIntent`/`CancelAll`/`Pause`/`Resume`/`Retry`/`ReleaseControl`, and `WaitForCompletionAsync`. The tools refuse rather than paper over: a refusal from the server is returned with its `IntentFailureEnum` and never retried, and command authority is never acquired as a side effect of a tool call. **One defect this surfaced:** `--profile robotics` shipped 36 tools that could not do anything. Every robotics tool resolves a named OPC UA session and only the connection tools can open one, so with no `Connect` in the catalogue the whole profile failed. A test had pinned the defect in place, asserting the profile did *not* contain `Connect` on the line below the `diagnostics` assertion requiring it. Each package now declares its own requirement in its registration extension rather than the host special-casing it, so an application embedding `Opc.Ua.Mcp.Robotics` directly gets a usable catalogue without reproducing the host's pairing. `Full` still takes the connection tools from the core package, so they are never registered twice. The replacement test states the invariant — a profile exposing session-scoped tools must expose the tools that open a session — rather than the instance. ## Making the MCP libraries trim and AOT clean *Merged from #4228.* `Opc.Ua.Mcp.Core` is now `IsAotCompatible` on `net10.0`, and the reflection the analysers then reported is gone. Turning the analysers on is the point: until now they were not running on these libraries at all — including on the tool library this branch adds. - **`OpcUaJsonHelper.Serialize`** went through `JsonSerializer` (IL2026 / IL3050). The conversion helpers in that class already reduce every OPC UA value to a closed set — `null`, the primitive scalars, `string`, `Dictionary<string, object?>` and lists thereof — so the JSON is now written directly with a `Utf8JsonWriter`. Output is unchanged, and that is asserted rather than assumed: `OpcUaJsonHelperTests.SerializeMatchesTheReflectionBasedSerializer` compares the new writer byte for byte against `JsonSerializer` for every shape the helpers emit, including all numeric widths, the custom-formatted scalars, nested sequences and strings that force the encoder to escape. One deliberate contract change: arbitrary object graphs are no longer accepted. Serializing them needs reflection, so `Serialize` now throws `NotSupportedException` naming the type instead of silently stringifying it. No call site in the repository passes anything outside the closed set. - **`McpSchemaFilters`** used `JsonSerializer.SerializeToElement` on a `JsonObject`; it now parses the node's own text, which needs no reflection. `Opc.Ua.Mcp.Diagnostics` is deliberately **not** marked. It depends on SharpPcap and on the reflective service-call dissection in `Opc.Ua.Core.Diagnostics`, neither of which is trim safe — which is why that package does not opt in either. Two improvements are kept regardless: `PacketDecodeTools` serializes `DecodedServiceCall` through a source-generated `JsonSerializerContext`, and `PacketCaptureTools.ListInterfaces` now carries the `RequiresUnreferencedCode` / `RequiresDynamicCode` annotations that `NetworkInterfaceEnumerator.ListLocalInterfaces` already declares, so the constraint is visible to callers instead of implicit. Nothing is suppressed. Marking an assembly `IsAotCompatible` only proves its own IL is clean; it says nothing about whether the library still functions once reflection is gone. So `Opc.Ua.Aot.Tests` references `Opc.Ua.Mcp.Core` and exercises it with JSON reflection disabled, stating what is and is not true today: - tool **results** serialize without reflection — this would fail outright if the `Utf8JsonWriter` change regressed; - tool **registration** still requires reflection, because the MCP SDK builds each tool's input schema by asking `JsonSchemaExporter` for a `JsonTypeInfo` of every parameter type. Without a source-generated context covering the tool signatures — and with the injected `OpcUaSessionManager` being described rather than treated as a service — that throws `NotSupportedException`. The second test asserts the throw. It documents the remaining gap and turns red the moment someone closes it, rather than leaving it to be found in a published binary. The package README says the same thing in prose. **Two pre-existing issues this surfaced, both outside this PR:** pulling `Opc.Ua.Mcp.Diagnostics` into an AOT publish reports `IL2046` / `IL3051` on `NicCaptureSource.StartAsync`, which is annotated `RequiresUnreferencedCode` / `RequiresDynamicCode` while the `ICaptureSource.StartAsync` it implements is not, and `IL2075` on `ServiceCallReassembler.GetCollectionCount` / `GetPropertyString`, which call `Type.GetProperty` on the result of `GetType()`. Happy to file these separately if useful. ## A generator that returned the value you opted out of *Merged from #4227. Fixes #4226.* `GetRandomNodeId(useBoundaryValues: false)` could still return `NodeId.Null` — one of the boundary values the same method offers when boundary values *are* requested. A caller that opted out should not receive one, and `GetRandomExpandedNodeId` builds on it and inherited the same behaviour. The `IdType.Opaque` branch is the practical path: `GetRandomByteString()` draws its length with `NextInt32(MaxStringLength)`, which is inclusive of zero, and a zero-length opaque identifier in namespace 0 satisfies `NodeId.IsNull`. With three namespaces and `MaxStringLength` at 10 that is roughly **one call in 130**. A numeric identifier of `0` in namespace 0 is null too, at around one in four billion. The draw is now repeated while the result is null and boundary values were not requested. Guarding the result rather than the individual branches covers every identifier type, including the numeric case, rather than only the one that happens to be reachable today. The `useBoundaryValues: true` path is untouched, so boundary values remain available when they are asked for. Four tests in `tests/Opc.Ua.Core.Tests/Types/DataGeneratorTests.cs` loop 20,000 draws, because at one call in 130 a single draw proves nothing. Against the **unmodified** generator they fail on draw 93 and draw 1346. Two of the four exist to keep the fix honest rather than to test the bug: `GetRandomNodeIdStillProducesEveryIdentifierType` catches a "fix" that simply stopped emitting opaque identifiers, which would satisfy the null assertions while silently narrowing the generator, and `GetRandomNodeIdWithBoundaryValuesStillYieldsThem` confirms the leak was closed rather than the feature removed. **How it was found:** an intermittent downstream test failure. A test server initialising a static `NodeId`-valued variable via `TestDataSystem.ReadValue` occasionally stored `NodeId.Null`; the encoder correctly writes that as JSON `null`, and a consumer asserting a value was present failed about one run in five — never reproducibly in isolation. It only surfaced under load because `UnsecureRandom.Shared` is a fixed-seed, process-global generator shared between `DataGenerator` and OPC UA transport jitter, so the number of draws preceding address-space initialisation depends on what else is running in the process. ## A NodeSet parent that lived outside the import batch *Merged from #4229.* `UANodeSetHelpers.LinkParentChildRelationships` builds its lookup table from **only the nodes in the current import batch**, links a child when the parent is found — and then cleared `instance.Handle` **unconditionally**, which is exactly where `Import` had stashed the authored `ParentNodeId`. A node whose parent lives in another NodeSet, or is owned by another NodeManager, therefore lost that parent **silently** at import, and nothing downstream could recover it. `RuntimeNodeSetNodeManager.CreateAddressSpaceAsync` already has a step that wires references to external node managers; the parent link never reached it. Such a parent is now recorded rather than discarded, and exposed through `UANodeSet.TryGetUnresolvedParentNodeId`. A parent that resolved normally inside the batch is **not** recorded, so a caller does not re-wire references that already exist. The record lives in a `ConditionalWeakTable`, so it does not keep an imported node alive and does not widen `NodeState` — whose `Handle` is a general-purpose slot a caller may legitimately use for its own purposes, and so is not a safe place to leave the value. `RuntimeNodeSetNodeManager` consumes it to report the one case that cannot be repaired automatically. In NodeSet2 the `ParentNodeId` attribute is a *hint*; the authoritative edge is the entry under `<References>`, which the reverse-reference pass already turns into an external reference. A node that names an out-of-manager parent but carries **no such Reference** materializes with no path from that parent, and no ReferenceType can be inferred to synthesise one — so it is reported as an authoring defect rather than guessed at or silently accepted. This is deliberately **not** a fix for a specific address-space symptom. It removes a silent loss of authored information and gives callers the means to act on it. Whether any particular orphaned-node report is caused by this path, or by namespace-index translation elsewhere, still needs separate diagnosis. **Changed while merging.** As authored in #4229 the accessor was `GetUnresolvedParentNodeId`, returning `NodeId?`. `NodeId` is a readonly struct implementing `INullable`, so that was a `System.Nullable<NodeId>` wrapped around a type that already carries its own null representation — which this repository excludes in favour of an `IsNull` check, and it asks for `TryGet` over casting on struct types. The tests correspondingly asserted `Is.Null` against a struct, which the testing rules also exclude. It is now `bool TryGetUnresolvedParentNodeId(NodeState, out NodeId)`, yielding `NodeId.Null` when there is no unresolved parent. The API is unshipped — it arrives with this branch — so nothing depended on the previous shape. Both tests were re-verified by mutation after the rewrite: disabling the record in `LinkParentChildRelationships` still fails `ImportKeepsAParentThatIsNotInTheSameBatch`. ## Dependencies - **OpenUSD to `0.7.0-alpha`.** All 11 issues we filed upstream are fixed, so the workarounds they justified are gone and the parts of the pump and generator samples that were blocked on them are implemented. - **ModelContextProtocol to `2.1.0`** across all five MCP tool libraries and the executable. No code changes were needed — the surface we use is unchanged in 2.x — but central package management required a cascade of pins, each commented where it sits. Both transports were verified against a running server rather than assumed. MCP 2.x is stateless (no `initialize`, no `Mcp-Session-Id`); an OPC UA session is application state addressed by a caller-supplied name, which fits that model but does assume process affinity, and that assumption is now written down rather than implied. ## Infrastructure The `code coverage` gate was measuring nothing on this branch and reporting success. It is fixed and now enforced, which immediately exposed real gaps — most usefully `RoboticsIntentJson`, which parses agent-supplied JSON (untrusted input from a language model) and had no tests at all. It is now at full line coverage in CI. Patch coverage is **90.20%** against a 75% floor. The `netstandard2.1` build leg was also failing. The MCP projects build as empty shells under a legacy `CustomTestTarget`, and the viewer sample referenced them on the target framework alone, so it tried to resolve a net10.0 shell from net8.0. The sample now derives MCP support from whether those projects are actually being built. ## From review A review pass over the branch found two real defects, both fixed. `RoboticsIntentJson` parses JSON written by a language model, so every malformed shape it can be handed is reachable input; it indexed pose arrays without checking their length and enumerated properties without checking their kind, so a short or wrongly-typed array escaped as `IndexOutOfRangeException` or `InvalidOperationException` rather than the argument error the tool descriptions promise the agent. Separately, `IntentOperationHandle` treated a zero `QueuePosition` as "not observed" and discarded it, so a handle kept reporting the last queued position after the server had said the operation left the queue. The pose validation immediately caught a latent test defect: the pallet scenario interpolated doubles into JSON with the current culture, so under a comma-decimal culture `[0.12, 0.16, 0.28]` became a six-element array. It passed only because that submission is expected to be refused for want of authority, so the wrong coordinates were never used. A third review finding — that a default-constructed `RobotIntentControllerState` hands out a null `NodeId` — was checked and rejected: `NodeId`, `QualifiedName` and `LocalizedText` are readonly structs here, so `default` is the null value and `IsNull` is `true`. The tests asserting that invariant are kept. A second review pass over the merged branch found two more, both fixed and both about an error surfacing as the wrong kind of failure rather than about a wrong result: - **Malformed agent JSON did not arrive as an argument error.** `RoboticsIntentJson` turns every *validation* failure into an `ArgumentException` — a wrong root kind, a missing property, a short pose array — but three `JsonDocument.Parse` calls and the NodeId parse were unguarded. A syntax error escaped as `JsonException` and an unparseable NodeId as `ServiceResultException`, which are the two shapes a language model is most likely to produce. That is the same defect class as the pose/kind checking above: a caller separating bad input from a genuine fault had to know to catch three unrelated exception types. Parsing now goes through one helper that reports a syntax error as an `ArgumentException` naming the argument, keeping the `JsonException` as the inner exception so the position is not lost, and the NodeId goes through `NodeId.TryParse` rather than `Parse`, collapsing its two failure modes into one. `MalformedIntentJsonIsRejected` had asserted `InstanceOf<JsonException>`, pinning the behaviour rather than the intended contract; it now asserts `ArgumentException` like every neighbouring test, and three tests cover the newly guarded paths. - **`ConditionalWeakTable.Add` throws on a duplicate key.** Not reachable today, but `TryGetUnresolvedParentNodeId` documents `Handle` as a general-purpose slot a caller may use for its own purposes — and a caller doing exactly that before re-importing into the same collection would take a hard throw from inside the import pipeline. Removing first makes the most recent import win. `AddOrUpdate` would say this more directly but does not exist on `netstandard2.0`. ## Deliberately not fixed here `FastenIntentDataType.Joint` is refused when the Server exposes no OPC 40450/40451 model, and a client cannot discover that before calling. That is **not fixable in this repository**: §5.4.2 makes `Joint` the defining mechanism while §11.3 says the parameters otherwise stand alone, and `IntentCapabilityDataType` declares capabilities at intent-type granularity with no member-level mechanism anywhere in the model. It is a specification gap, raised as S4 on [marcschier/opcua-drafts#60](https://github.com/marcschier/opcua-drafts/pull/60) rather than worked around with a bespoke mechanism the spec does not define. ## Validation `dotnet build UA.slnx -c Release` on the merged branch — **0 errors, 0 warnings**, with the AOT analysers now running over `Opc.Ua.Mcp.Core`. | Suite (net10.0) | Tests | |---|---| | `Opc.Ua.Core.Tests` | 4247 (86 skipped) | | `Opc.Ua.Robotics.Tests` | 540 | | `Opc.Ua.OpenUsd.Tests` | 909 | | `Opc.Ua.Tools.Tests` | 419 | | `Opc.Ua.Server.Tests` — `RuntimeNodeSet` | 94 | | `Opc.Ua.Robotics.Intent.Tests` | 22 | | `Opc.Ua.Aot.Tests` — `McpAotTests` | 3 | 0 failures. `Opc.Ua.Tools.Tests` rose from 409 to 419 with the serializer-equivalence tests from #4228 and the malformed-JSON tests from the second review pass, and `Opc.Ua.Core.Tests` covers the `DataGenerator` tests from #4227 and the import tests from #4229. `UANodeSetHelpers` also passes on **net48** (5/5). The MCP agent-usability grader scores Core 100 / Full 100. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 Copilot-Session: a0718e8a-a158-45c5-929d-e1d6c0982431	2026-08-11 13:03:36 +02:00
Marc Schier	4e0b82af5f	Add IServerContext and move server startup tasks onto it (#4193) ... ## Stacked on #4188 Base is `marcschier/server-seam-analysis`, not `master`. `IServerInternal.cs` and `ServerInternalData.cs` both diverge from master on that branch (chunk 4c removed the two `object` locks), so branching off master would conflict on both files *and* design `IServerContext` against a stale interface. GitHub retargets the base to `master` automatically when #4188 merges. **Review #4188 first.** Closes part of #4184. ## What this does `IServerInternal` is a service locator: 56 members, 30-odd of which hand out one of 20+ subsystems. A component that only needs to observe the server and act on it has to take the whole thing, and no signature tells a reader which parts a component actually touches. This adds `IServerContext` — the ambient view of a running server — and moves `IServerStartupTask` onto it. ```csharp public interface IServerContext { ServerState CurrentState { get; } ServerObjectState ServerObject { get; } IServiceMessageContext MessageContext { get; } ServerSystemContext DefaultSystemContext { get; } ServerSystemContext CreateSystemContext(ISession session); T? FindPredefinedNode<T>(NodeId nodeId) where T : NodeState; IEnumerable<T> FindNodeManagers<T>() where T : class; ValueTask ReportEventAsync(IFilterTarget e, CancellationToken ct = default); ValueTask CloseSessionAsync(OperationContext context, NodeId sessionId, bool deleteSubscriptions, CancellationToken ct = default); ValueTask DeleteSubscriptionAsync(uint subscriptionId, CancellationToken ct = default); void UpdateServerDiagnostics(Action<ServerDiagnosticsSummaryDataType> update); } ``` Nothing on it returns a subsystem. A component that needs one takes it as a constructor dependency, which states the requirement instead of hiding it behind an ambient handle. `IServerInternal : IServerContext`, so every existing handle already satisfies the new seam and consumers migrate one at a time. ## The member set was measured, not guessed Every member of `IServerInternal` was counted by read site across `src`, `samples` and `tests`, split by area. `Ext` is non-test production consumers, which is what decides whether something belongs on an ambient interface. | Member | InServer | OtherSrc | Sample | Test | **Ext** | On `ISystemContext`? | |---|---:|---:|---:|---:|---:|---| | `NamespaceUris` | 63 | 50 | 34 | 131 | **84** | yes | | `Telemetry` | 58 | 23 | 35 | 5 | **58** | yes | | `MessageContext` | 2 | 8 | 7 | 11 | **15** | no | | `TypeTree` | 42 | 3 | 11 | 10 | **14** | yes, as `TypeTable` | | `NodeManager` | 66 | 7 | 6 | 74 | **13** | no | | `DefaultSystemContext` | 21 | 4 | 1 | 7 | **5** | no | | `IdentityRegistry` | 1 | 4 | 1 | 1 | **5** | no | | `SubscriptionManager` | 23 | 0 | 0 | 17 | **0** | no | | `SessionManager` | 21 | 0 | 0 | 15 | **0** | no | | `RequestManager` | 10 | 0 | 0 | 23 | **0** | no | | `AggregateManager` | 12 | 0 | 0 | 2 | **0** | no | | `ModellingRulesManager`, `ConformanceUnitsManager`, `DiagnosticsEnabled` | 0 | 0 | 0 | 0 | **0** | no | Two things fall out. The highest-traffic members are already carried by `ISystemContext`, which `ServerSystemContext` copies off the server at construction — so most of those 278 `NamespaceUris` reads are callers reaching through the server for something they already hold. And the subsystems with zero external demand are exactly the ones that should be injected rather than published. The check that matters: the eight `IServerStartupTask` implementations in `Opc.Ua.Redundancy.Server` are the heaviest real consumer of the ambient handle, and between them they touch **eight distinct members**. That is the shape the interface is cut to. ## Commits ### `e4a912eb` Remove seven unused members from `IServerInternal` The deletion test, applied before anything is designed against the interface. | Removed | Evidence | |---|---| | `CloseSession(OperationContext, NodeId, bool)` | zero call sites; sync-over-async; already obsolete | | `ReadServerDiagnostics<TResult>` | zero production sites — **added one commit earlier in #4188 for symmetry and never called** | | `ModellingRulesManager` | property never read; `ServerInternalData` owns the lifetime | | `ConformanceUnitsManager` | same | | `DiagnosticsEnabled` | façade over `IDiagnosticsNodeManager.DiagnosticsEnabled`, which is what callers use | | `Status` | one site, itself inside an obsolete method | | `ServerDiagnostics` | only reachable through the diagnostics methods that replaced it | `EndpointAddresses` was kept — `Opc.Ua.Di.Tests` uses it to resolve an endpoint URL. Thin, but real. The only build fallout was three lines in one test assembly. `Opc.Ua.Redundancy.Server`, `Opc.Ua.Redundancy.Kubernetes`, `Opc.Ua.Gds.Server.Common`, `Opc.Ua.WotCon.Server`, `Opc.Ua.PubSub.Server`, `Opc.Ua.Di.Server`, `ConsoleReferenceServer` and `Quickstarts.Servers` all built untouched. That is the evidence the members were dead. ### `ffcc80b9` Add `IServerContext` Additive. `ServerInternalData` implements it; no caller changes. ### `e8c4e15f` Hand startup tasks `IServerContext` Nine implementations, `OpcUaServerHostedService`, and the test call sites. ### `1b4de97a` Drop the `IServerStartupTask` migration-guide section Review feedback. `IServerStartupTask` does not exist in `master378`, so it is a 2.0-only concept and nobody migrating from 1.5.378 can own an implementation of it. The **Removed members on `IServerInternal`** section stays - those members *are* in master378 - and is now self-contained. ### `2ab7d689` Merge `marcschier/server-seam-analysis` Picks up the base branch including its merge of master. Textually clean, no conflicts. ### `6a650f12` Migrate `RobotIntentHostStartupTask` to `IServerContext` The merge was textually clean but **semantically broken**: master's Robot Intent feature (#4165) added a tenth `IServerStartupTask` implementation written against the old `IServerInternal` signature, so it no longer satisfied the interface. Its body walked `server.NodeManager.AsyncNodeManagers` and type-tested each entry for `RobotIntentNodeManager` - exactly the pattern `FindNodeManagers<T>()` replaces - so it migrated the same way as the other nine. `FindNodeManagers` also searches the synchronous registry and de-dupes by reference, so a manager registered on both faces still starts its intent controller hosts exactly once. ## Three findings from migrating real consumers **`MessageContext` is not derivable.** The design had callers use `DefaultSystemContext.AsMessageContext()`, on the reasoning that a message context can be built from a system context. It can't: that conversion carries the namespace tables and the factory but **not** `MaxStringLength`, `MaxArrayLength` or `MaxByteStringLength`. Four replication paths in `Opc.Ua.Redundancy.Server` would have silently fallen back to default decoding limits — a component quietly accepting more than the server is configured to. It is a real member, and the interface documents why. **`new`-shadowed members split the mock surface.** `IServerInternal` first redeclared `DefaultSystemContext` with a narrower type and `CurrentState` with a setter. That compiles, but a shadowed member is a *second interface slot*: production code calling through `IServerContext` read a slot that every existing `Mock<IServerInternal>` leaves unstubbed. Seventeen tests went from passing to `NullReferenceException` **while still compiling**. The narrower typing bought no encapsulation anyway — the object really is a `ServerSystemContext` and any caller can cast — so both shadows are gone. `CurrentState` is read-only on the ambient interface and the one site that assigns it uses the concrete type, as `GetStatus` already does. **`IServiceProvider` is deliberately *not* on `OnServerStartedAsync`.** The plan sketched it. All nine implementations already receive their dependencies through their constructors from the builder extensions, so a container on the callback would have replaced one service locator with another. ## `FindNodeManagers<T>` Two tasks walked `server.NodeManager.NodeManagers` and type-tested each entry for `ILocalAddressSpaceSource`; two more reached for a specific manager to cast it to a capability. Asking for the capability says what the caller wants; asking for the registry says how to find it. ```csharp // before foreach (INodeManager nodeManager in server.NodeManager.NodeManagers) { if (nodeManager is not ILocalAddressSpaceSource source) { continue; } ... } // after foreach (ILocalAddressSpaceSource source in server.FindNodeManagers<ILocalAddressSpaceSource>()) { ... } ``` It searches both the asynchronous and synchronous registries: `ToSyncNodeManager()` returns `this` when a manager already implements `INodeManager`, so the two lists overlap and a capability may sit on either face. ## Tests Mock harnesses are **wired, not stubbed** — they return the node managers and contexts the tests already build, so the tests keep asserting against real behaviour rather than against a mock that returns whatever is convenient. Four new tests cover `CreateSystemContext` (identity/locale propagation, null guard), the explicit `DefaultSystemContext` identity, and `FindPredefinedNode` before the address space is bound. | Suite | net10.0 | net48 | |---|---|---| | `Opc.Ua.Redundancy.Server.Tests` | 573 passed | 573 passed | | `Opc.Ua.Redundancy.Kubernetes.Tests` | 97 passed | n/a (net8/net10 only) | | `Opc.Ua.Server.Tests` | 4028 passed, 1 failed | 225 passed, 1 failed (filtered) | The single failure is `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a local certificate-store problem. Verified it reproduces unchanged at HEAD with these changes stashed — it is not caused by this PR. All twelve dependent assemblies and samples build clean on both TFMs, zero warnings. ### Re-verified after the base merge (`6a650f12`) `Opc.Ua.Server`, `Redundancy.Server`, `Redundancy.Kubernetes`, `Robotics.Server`, `WotCon.Server`, `Di.Server`, `PubSub.Server`, `Gds.Server.Common` and `XRegistry.Server` all build clean on net10.0. `Opc.Ua.Robotics.Tests` 491 passed, `Opc.Ua.Redundancy.Server.Tests` 573 passed, `Opc.Ua.Redundancy.Kubernetes.Tests` 97 passed. ### CI status: 119 passed, 3 failed - none caused by this PR | Failing job | Why it is not this PR | |---|---| | `SourceGeneration_Core_Tests_windows` (net48) | `GenerateAndCompileDesignFileAsync(..., CSharp11, ...)` fails with `CS9058: Feature 'Lock object' is not available in C# 11.0` because generated `Opc.Ua.NodeStates.g.cs` emits `System.Threading.Lock`. **Identical failure on base #4188 (build 16737)**, which contains none of these commits. This PR touches no source-generation file. | | `SourceGeneration_Core_Tests_windows` (net10.0) | Same root cause; also failing on base #4188. | | `PubSub_Eth_Tests_windows` (net48) | Agent DNS outage: `NuGetToolInstaller` died with `ERR:getaddrinfo ENOTFOUND dist.nuget.org`. The job aborted before restore/build, so **the test step never executed**. Infra flake; needs a re-run, not a code change. | Both `SourceGeneration.Core` failures belong to the base branch and should be fixed there rather than papered over here. ## Correction to the recorded plan Chunk 7 (the frozen bind phase) was recorded as the largest mechanical footprint remaining and the one most likely to stall. It isn't: **each of the 12 `Set*` mutators has exactly one call site**, in the `StandardServer` wiring, plus one sample. It is a ~13 call-site change. The 268-file figure that produced that assessment counted files *referencing* `IServerInternal`, which chunk 7 never has to touch, because the interface survives and only the mutators move off it. ## Docs `docs/MigrationGuide.md` gains one section: the removed `IServerInternal` members with their replacements, plus the note that `MessageContext`, `DefaultSystemContext`, `CurrentState`, `ServerObject`, `ReportEventAsync`, `CloseSessionAsync`, `DeleteSubscriptionAsync` and `UpdateServerDiagnostics` moved *down* to `IServerContext` and stay reachable through `IServerInternal` unchanged, so no call site has to move. An earlier revision also documented the `IServerStartupTask` signature change there. That was dropped after review (1b4de97a4): `IServerStartupTask` does not exist in `master378`, so it is a 2.0-only concept and nobody migrating from 1.5.378 can own an implementation of it. The rationale for the ambient view - including the `AsMessageContext()` decoding-limit trap - lives on the `IServerContext` and `IServerStartupTask` XML docs instead, which is where a 2.0-only concept belongs. ## Reviewer questions - `IServerContext` is 11 members, not the 9 the design targeted. `MessageContext` and `FindNodeManagers<T>` were added because migrating real consumers showed the alternatives were wrong (silent decoding-limit loss) or a pass-through (`NodeManager.NodeManagers` plus a type test at every call site). Reasonable? - `IServerInternal : IServerContext` keeps the fat interface alive on purpose — it is what makes this a signature change rather than a 268-file rewrite. It only shrinks once the DI migration lands. The 6a deletions are unconditional, so even a full stall leaves it seven members smaller. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63	2026-08-10 21:14:09 +02:00
Copilot	404517c6e8	[MigrationAnalyzer] Generate public collection shims to unblock incremental 1.5.378 → 2.0 migration (#4214) ... # Description MigrationAnalyzer-generated `<Type>Collection` shims were emitted as `internal`, which breaks consumers exposing these types in public APIs during migration. This change makes generated shims `public` while preserving `[Obsolete]` + `UA0002` guidance. - **Generator output access level** - `MigrationGenerator` now emits: - `public sealed [Obsolete] class <Name>Collection : List<TElement>` - No behavior changes to element-type resolution, deduplication, or diagnostics (`MIG01`). - **Targeted generator test updates** - Updated assertions in `MigrationGeneratorTests` to validate `public sealed class ...` output for: - well-known overrides, - semantic lookup for model types, - semantic lookup for built-in and primitive element types. - **Migration docs alignment** - Updated migration analyzer docs to reflect public shim generation and removed outdated internal-only guidance. ```csharp [global::System.Obsolete("... (UA0002)")] public sealed class Int32Collection : global::System.Collections.Generic.List<int> { public static implicit operator global::Opc.Ua.ArrayOf<int>(Int32Collection? value) => value is null ? default : value.ToArrayOf(); } ``` ## Related Issues - OPCFoundation/UA-.NETStandard#4205 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [ ] I have added all necessary documentation. - [ ] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #4205 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-08-10 16:28:56 +00:00
Marc Schier	e05a820c91	Removal of the exposed diagnostics and node locks (#4188) ... ## What this is Design analysis for `Opc.Ua.Server`, the two rounds of design exploration behind it, and the first two implementation chunks it recommends. Draft: the analyses are ready to read, but the *conclusions* deserve discussion before the larger phases proceed. Later chunks stack onto this PR as they land. | File | Kind | Lines | |---|---|---:| | `plans/NodeManagerAnalysis.md` | Evidence — the node-manager plugin surface | 488 | | `plans/ServerRuntimeAnalysis.md` | Evidence — the server runtime cluster | 576 | | `plans/ServerDesignExploration.md` | Design round 2 — delivery strategy | 1,394 | | `plans/ServerDesignExplorationAlt.md` | Design round 1 — four interface designs | 2,119 | | `src/Opc.Ua.Types/State/NodeState.cs` | **Code** — node self-guards attributes | | | `src/Opc.Ua.Types/State/BaseVariableState.cs` | **Code** — value accessors take the lock | | | `src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs` | **Code** — 6 external locks removed | | | `tests/…/NodeStateAttributeConcurrencyTests.cs` | **Code** — concurrency safety net | 449 | `docs/` is untouched — it holds shipped feature documentation, and none of this is shipped API. ## The analyses Counted mechanically over `src`, `samples` and `tests`; **every figure is reproducible** from commands printed in each document. **Node manager surface.** `AsyncCustomNodeManager` declares **93 `virtual`/`abstract` members** (80 distinct names). Across the repository: **112 override sites, 25 distinct names ever overridden, 55 of 80 never**. Six carry nearly all real use. For scale, `ReferenceNodeManager` is 6,373 lines and overrides **8**. **Server runtime.** Seven findings, **two of which recommend no change**. Findings 1–5 are filed as #4183–#4187. The headline is an `object`-typed sync root published on three public interfaces and taken in **88 `lock` statements across 7 files**, including a shipped sample. **What is deliberately not flagged.** `IStandardServer` — 9 members over 5,575 lines, 16 of 37 virtuals used — is identified as the reference shape to preserve. `IApplicationInstance` and the already-`[Obsolete]` `Stop()` are examined and left alone. ## The design exploration **Round 1** explored four alternative interfaces in parallel under the current back-compat constraint. Its value is the convergence: four designers who never saw each other's work independently arrived at a 6–9 member `IServerContext` replacing the 57-member locator, removal of the `object` node-manager handle, elimination of the `Processed` protocol, and `IHistorianProvider` as the pattern to copy. **Round 2** re-runs it with back-compat lifted and asks how to deliver: full rewrite versus side-by-side fork. **Both option designers independently declined to recommend their own assignment.** Three structural facts were verified: 1. **The fork line already sits at `IAsyncNodeManager`** — old and new node managers can coexist with zero runtime changes, so a fork duplicates ~10% of the assembly, not all of it. 2. **`[OpcUaShim]` cannot shim an inheritance surface** — honest auto-fix ceiling for the authoring surface is **~10%**, against ~70% for the shipped analyzers. 3. **A second lock leak exists** — `NodeState` takes `lock (this)` under a `CA2002` suppression whose own comment documents the contract. Recommendation: **neither pure option**, but a staged path reaching the rewrite end state incrementally, gated on migrating the five framework-internal node managers that today make it impossible for the old base to ever freeze. ## Implementation: NodeState now guards its own attributes This removes the prose-only contract in which `NodeState` took `lock(this)` and every external caller was required to take `lock(source)` on the node instance. The code already carried a TODO describing exactly this fix and noting it required updating every external site. - The node self-guards rather than relying on callers. `BaseVariableState` gains a **fully private** `System.Threading.Lock` covering its value fields; `NodeState`'s other collections keep their existing private locks. - All **6** `lock(this)` sites and both `CA2002`/`RCS1059` suppressions are gone. - `Value`, `Timestamp` and `StatusCode` on `BaseVariableState` take the lock too — without that, a direct property write raced an attribute read once the external lock was gone. - **6 of 9** external sites removed. Five were already redundant (`GetReferences`, `GetChildren`, `ReferenceExists` and `SetAreEventsMonitored` guard themselves; `FindMethod` reaches its state via `GetChildren`); the sixth was an attribute read now covered by the node's own lock. - Attribute read/write deliberately take **no** node-wide lock: both dispatch to caller-supplied handlers (`OnReadValue` and siblings) that may take locks of their own, and holding a lock on the node across one gives the node no fixed place in the lock order. - **3 deliberately retained and annotated in code**: two `CreateBrowser` calls, which are not internally synchronized (tracked in #4216), and one in `CustomNodeManager`, whose class is slated for deletion so refactoring its lock would be wasted work. ### No lock is exposed anywhere, to anyone An earlier revision put the attribute lock on `NodeState` as `private protected`, which still published a lock-sharing contract on the inheritance surface. Review pushed back and it is gone: `BaseVariableState` was the only user, so the lock moved there as a plain `private readonly Lock`. There is no member for a derived type to call, in this assembly or any other. `BaseVariableValue` was the last public leak — it handed out the lock guarding its value through a `public Lock` property, so any caller could hold it across a call back into the stack. That property is removed: - Derived value classes (what the source generator emits for every structure variable) synchronize through a `protected EnterLock()` / `ExitLock()` pair. Callback helpers were the first choice, but the generated bodies pass `ref Variant` / `ref StatusCode` / `ref DateTimeUtc`, and `ref` arguments cannot be captured by a lambda. The lock *object* stays private either way. - `NodeStateTemplates` emits the new form. - `ServerInternalData`, the one cross-assembly caller, was reading the lock back off a value it had itself constructed with that lock. It now takes `m_diagnosticsLock` directly. `NodeBrowser.DataLock` remains `protected` and is tracked in #4215: a subclass extending `Next()` legitimately needs its enumeration state atomic with the base browser's, so the fix needs a base-owned acquisition rather than a deletion. ### Migration coverage for the removed lock members None of the removed lock members get an `[Obsolete]` shim, and the migration guide now says why: a shim would have to be `object`-typed to keep old call sites compiling, and on .NET 9+ `lock (o)` on a statically-`object` reference is `Monitor.Enter`, which does **not** interlock with `Lock.Enter()`. It would compile, run, and silently synchronize nothing — strictly worse than a compile error. Re-adding a member to a downstream-implemented interface is also a break in its own right on `net48` / `netstandard2.0`, where there are no default interface methods. Instead, every removed member has a named replacement on the migration path: | Rule | Covers | Replacement it names | |---|---|---| | `UA0024` | `IServerInternal` / `ISession` / `ISubscription` `DiagnosticsLock`, `DiagnosticsWriteLock` | `UpdateServerDiagnostics(...)` / `UpdateDiagnostics(...)`, `ReadServerDiagnostics` / `ReadDiagnostics` | | `UA0025` | `ILocalNode.DataLock`, `Node.DataLock` | a lock the caller owns; the node guards its own state | | `UA0026` | `BaseVariableValue.Lock` | a lock passed to the `BaseVariableValue` constructor | Each rule fires on sources still written against 1.5.378, where the compiler only says a member is missing. `docs/MigrationGuide.md` gains a section per removal with before/after. ### The tests earned their keep The four concurrency tests were landed **first**, against the unmodified code, because removing a synchronization contract cannot be verified by the compiler and the existing `NodeStateHandlerConcurrencyTests` covers a different concern (racing a handler assignment against a write, not reader/writer exclusion). They now run with **no external locking**, which is what demonstrates the node guards itself. They caught two real problems, and one of them was mine: - The `Value`/`StatusCode`/`Timestamp` setters bypassed the new lock — a genuine race. - One test asserted an invariant the design does not offer. Two property setters are two lock acquisitions, so that pair was **never** atomic; it only passed originally because the test held a lock across both. It now uses a single attribute write carrying a whole `DataValue`, which genuinely is atomic and is the path the server uses. A third issue was found and fixed before it shipped: the reader loops gated on `!writer.IsCompleted`, so a reader scheduled after the writer finished performed zero reads — failing two runs in three on `net48`. Converted to `do…while`; now five consecutive green runs. **Verified:** `Stack.State` 116 passed on `net10.0` *and* `net48`; the concurrency category green 5/5 consecutive on `net48`; `Opc.Ua.Types.Tests` 8,456 passed; the server node-manager suite 1,205 passed, including `NodeManagerLifecycleTests` and `AsyncCustomNodeManagerTests` which exercise the changed paths. ## Two corrections to my own published figures Both were found while starting implementation, and both are recorded on #4183. **`ForceDiagnosticsScan()` was overstated.** The analysis claimed the `DiagnosticsWriteLock` getter *"triggers a full address-space diagnostics rescan"*. It is in fact a single flag assignment (`DiagnosticsNodeManager.cs:700-703`) consumed by the read paths and the periodic timer. The defect stands — a side-effecting property getter with a racy write on a synchronisation path — but its cost was misstated. **The contract was 15 sites, not 19.** A naive search matched ten comment and `#pragma` lines that *mention* `lock(source)` while explaining it. The documents now carry a counting command that excludes comment matches. A third finding expanded scope the other way: the `object`-typed lock leak is **repo-wide**, with ten such members across `Opc.Ua.Core`, `Opc.Ua.Types` and `Opc.Ua.Server` — including `Node.DataLock => this` and `ApplicationConfiguration.PropertiesLock => m_properties`, the same defect class as `Subscription.DiagnosticsLock`. ## Caveats stated in the documents - **Nothing in the designs is implemented or benchmarked.** Every performance claim across all seven designs is a design intention, not a measurement. - The larger phases are a **major undertaking against a live plugin API**; the documents say so rather than letting the neatness of the proposals imply otherwise. - The recommended sequencing puts low-risk, self-contained steps first and notes they are **worth doing regardless** of which strategy is eventually chosen. - Round 2 notes the two option designers' cost estimates are **not independent of their assignments**. ## Review guidance The two analyses make factual claims checkable by running the included commands. The design explorations are exploratory and need not be agreed with to merge. The code change is small, test-backed, and independent of which delivery strategy is eventually chosen. Reasonable outcomes include merging as-is, merging only the analyses and the lock fix, or disagreeing with the recommendation while keeping the evidence. --- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041 Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63	2026-08-10 06:47:50 +02:00
Marc Schier	0612a43fe4	Pluggable cryptography and hardware-held private keys (#4192) ... Makes the stack's cryptographic operations replaceable - by another library, an offboard service, or hardware (TPM 2.0 / HSM / PKCS#11 token / cloud KMS) - with no performance cost in the default configuration, and extends this to certificates so a private key need never exist in process memory. Also fixes three pre-existing defects found while validating: STACKGEN001 erroring for every compilation the stack generator was merely loaded into, certificates failing to load from paths past MAX_PATH on Windows, and the MonitoredNode2 tests starving the thread pool. Fixes #4190	2026-08-09 12:40:26 +02:00
Marc Schier	2b44fcad22	Robot intent: a task-level command API with a Part 10 lifecycle (#4165) ... # Description Implement the draft **OPC UA — Robot Intent** companion model end to end: task-level motion verbs, a Part 10 lifecycle, a server hosting surface, a client, two samples and a live integration suite. It replaces the opt-in convention API merged in #4127 with an implementation generated from a companion information model. ## Why OPC 40010-1 *Robotics* describes robot **topology** in detail and defines **no motion verbs at all** — its entire actuation surface is `Start`, `Stop` and loading a named program. A conformant client can discover everything about a robot's construction and cannot ask it to move anywhere. #4127 filled that gap with ten verbs in an application-owned namespace, resolved by BrowseName, and the file said what it was: *"opt-in, explicitly non-normative industrial operation conventions"*. That contribution established a **vocabulary**. It established no **lifecycle**, and the lifecycle is the harder half. A motion takes seconds; a pick takes a minute. OPC 10000-4 §5.12.2 discards a method result when the Session ends *"independent of the task actually performed at the Server"* — so a synchronous motion method does not merely time out, it **loses the outcome of work that has already physically happened**. OPC 10000-10 §4.1 gives the OPC Foundation's own resolution: a Method performs a calculation, a **Program** runs a batch process or a machine tool part program. So `SubmitIntent` returns as soon as the intent is *admitted*, and what it returns is a NodeId — an `IntentOperationState` (a Part 10 program instance) the client subscribes to for progress and reads for the result. ## What changed **Model and shared code** — `Opc.Ua.RobotIntent.NodeSet2.xml` is source-generated in `Opc.Ua.Robotics`, so the enums, the polymorphic intent structures, `IntentOperationState : ProgramStateMachineState`, the method states and the typed clients derive from the model rather than being hand-written. Its only `RequiredModel` is the base UA namespace, so a server can adopt it without OPC 40010 or DI. `PoseMath` implements the specification's Annex C conversion between its unit quaternion and the core `ThreeDFrame` — including the `asin` clamp that stops a pole orientation becoming a domain error, and the non-negative-`w` representative that makes two servers agree on four numbers. `FrameTree` composes transforms along the frame tree. **Verbs are a DataType hierarchy, not one Method each.** A single submission and a mission step are then the same shape, and a new intent is a subtype rather than a new method — which is what `AddOperation<TRequest,TResponse>` was working around. **Server** — `IntentControllerHost` owns admission in the specification's order, the queue with PLCopen buffer modes, blending, cancellation *with the server's right to refuse*, missions with a committed base and a revisable horizon plus an IEC 61131-3 step graph, safety-aware refusals, real-time channel brokerage, and the capability declaration. All thirteen Methods are wired; command authority and channel leases are released when the holding Session closes; every client-supplied NodeId is validated against the controller being commanded (clause 11.3). `RobotIntentNodeManager`, the fluent `IIntentControllerBuilder` and `AddRobotIntent`/`ConfigureRobotIntent` make it reachable, and `RobotIntentFacetCalculator` computes which of the specification's facets an instance actually satisfies. **Client** — `RobotIntentClient` discovers `Server/RobotIntent/Controllers` and reads what a robot accepts; `IntentOperationHandle` subscribes and completes on a terminal state, recovering the result after a reconnect because clause 6.7 promises it survives; `CommandAuthorityLease` and `RealTimeChannelLease` manage the two exclusive resources; fluent builders cover every intent and mission. **Samples** — all robotics samples move under `samples/Robotics/`. `IntentEnabledRobot` is one UR5e-style offset-wrist arm on a bench, declared completely enough to show what a conformant server publishes, with a simulated safety source so the clause 10.4 refusals are demonstrable. `IntentViewerClient` turns a **click on a target prim in the OpenUSD viewport** into an intent and watches the arm execute it; it also runs headless, which is how CI exercises it. **Intents execute serially**, which satisfies every `BlockingMode` constraint by construction: the specification forbids beginning a `Single` or `Hard` intent while another executes and merely *permits* `None` and `Soft` to overlap. The convention API (`RoboticsOperationConventions`, `RoboticsOperationsBuilders`, `RoboticsOperationsClient`) is removed. ### On safety — what this deliberately does *not* claim The interface is **non-safety-rated**, and that is a property of the technology rather than a scoping choice. OPC 10000-15 carries cyclic safety data from a SafetyProvider to a SafetyConsumer; the consumer's `RequestSPDU` holds an identifier, a monitoring number and one octet of explicitly **non-safety** flags, so a caller has no channel through which to supply safety-rated arguments. Every safety fieldbus (PROFIsafe, CIP Safety, FSoE, openSAFETY) expresses a safety command as a **continuously asserted cyclic signal**, because the integrity argument rests on the fail-safe state that follows when assertion *stops* — and a Method call has no defined behaviour when it stops being called. What the host does instead is **observe and refuse**: `UpdateSafetyState` is how the application reports what the safety system is enforcing, and admission then refuses on the same values a client can read, so a refusal is explainable from the address space rather than from Server-internal state. It may never *instruct* the safety system, and no Method here commands a safe motion function, changes an operational mode or clears a stop. ## Defects this found in the specification, fixed upstream Implementing a specification is the only way to find out whether it can be implemented. Seven defects went back to [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) rather than being worked around here: - **Refusal was unobservable.** Clause 6.2 orders six distinct refusals and clause 5.8 calls the failure set "small and diagnosable on purpose" — but `SubmitIntent` returned only `IntentId` and `Operation`, so a client could see *that* something was refused and never *which*. It now returns `Accepted`, `Failure` and `Message`, and clause 6.2 states that a refusal returns `Good` rather than substituting a Bad `StatusCode`. - **The Part 10 promotion was illegal.** `IntentOperationType.ProgramDiagnostic` was declared as a Property of `PropertyType`; OPC 10000-10 declares it a Variable of `ProgramDiagnostic2Type` reached by `HasComponent`. The declaration added a *second* member beside the inherited one instead of promoting it — which is precisely why an earlier version of this branch had deleted it. Correcting the TypeDefinition made it generate cleanly with **no change to the curated Core**. - **Clause 9 said nothing about the Method surface.** A server could declare `MissionsSupported` true and omit `SubmitMission` entirely, because the mission and channel Methods are Optional. This implementation did exactly that until an integration test caught it. A fourth honesty rule and a table now fix which Methods each declaration implies. - **Conformance was not machine-readable, and two implementations disagreed.** Clause 12 defined conformance in terms of facets and gave a Server nowhere to publish which ones it satisfied, so every client had to re-derive Table 12.2 from the address space. This repository proved why that fails: the server''s `RobotIntentFacetCalculator` and the client''s `RobotIntentRules.DeriveFacets` each implemented clause 12.2 independently and **disagreed on essentially every row** — the client''s `Blending` was `BlendingSupported` alone where the server also requires all four blending buffer modes, its `QueuedIntents` was `MaxQueueDepth > 0` where the server also requires `Buffered`. A client and server from the same release answered "does this controller conform?" differently, and both were reading the specification correctly, because several rows are behavioural and no amount of browsing settles them. `IntentCapabilitiesType` now carries **`SupportedFacets`**, exactly as OPC 10000-5 does with `ServerCapabilitiesType.ServerProfileArray`, RI-Base requires it, and clause 12.2 separates structural requirements a Server shall meet before listing a facet from attested ones governed by clause 9. - `WaitIntentDataType.Signal` was unbounded where clause 11.3 has to check it; §11.3 gained a table fixing the expected type of every NodeId-valued member; §5.7.0 gave `Ready`/`ActiveIntent`/`ActiveMission` normative meaning; §6.9 bounded `RequestedLease`; §6.5 and §6.4 now say what a server that cannot differentiate `StopMode` must do, and which stop a superseded intent gets. ## Bugs the tests and reviews caught Each of these would have been silent in the field: - **The execution engine was never wired into the public API.** `IntentControllerBuilder.RegisterAsync` built the address space and never started an `IntentControllerHost`, so through the documented `AddRobotIntent` → `ConfigureRobotIntent` path **every Method returned `BadNotImplemented`** and the registered `IIntentExecutor` was never resolved. A browsable address space is not a working robot interface, and only a live integration test showed the difference. - **NodeId scoping failed open on an empty index.** A controller advertising `CallProgramIntentDataType` with no published `ProgramType` accepted an *arbitrary* NodeId as `CallProgram.Program` — and `CallProgram` runs code the server holds. - **Blending completed the predecessor at submission time** and abandoned its motion, telling the client it succeeded and publishing an all-zero pose at the origin as a genuine `AchievedPose`. - **Mission base immutability compared only identifiers**, so a client could keep the ids and swap the *motion* of an already-committed step. - **`FinalResultData/Result` was never registered** with the node manager: Browse showed it, Read returned `BadNodeIdUnknown` — so clause 6.7's promise to a Part 10 client did not hold. - `DisposeAsync` disposed primitives out from under in-flight executor code; re-using a terminated `IntentId` destroyed the retained result and duplicated a NodeId; neither side registered the Robot Intent encodeables, so `Pose3DDataType` did not decode; host startup was hooked to a task that ran in only one hosting shape. - The result was published **after** the state went terminal; capabilities were resolved against a stale namespace table; an empty `ContentFilter` arrives as an empty element array rather than a null filter, so testing only for null made every unconditional mission transition silently untaken — a mission would run its first step and stop. ### Found by a later test-quality audit, after the above was written An audit of the test suite itself found five tests whose **names** asserted a normative requirement while their **code** asserted something weaker or nothing at all. The worst **fabricated the answer it checked**: a helper caught a decode failure and invented "5 supported intents, 6 axes, all facets true", so six capability-honesty assertions across four tests could pass while reading nothing from the server. The fallback is gone and the suite passes with honest reads. Chasing the facet gaps it reported led to the clause 12 defect above. Two further defects followed from completing that work: - **`RI-Interop-40010` was never computed**, and the code documented the omission as impossible to fix "because the calculator receives only a Robot Intent controller, not the linked OPC 40010 model". That was wrong: Annex B's structural requirement is an **inverse** `HasIntentController` reference, reachable from `IntentControllerState` — and the integration suite was already browsing exactly that. A server implementing Annex B could not claim Annex B. - **The published claim could go stale.** `SupportedFacets` was a snapshot taken during `RegisterAsync`, so a reference attached afterwards left it wrong. It is now bound through `BindRead` and recomputed on read, so the claim tracks the address space by construction rather than by discipline — which matters for anyone bolting Robot Intent onto an existing OPC 40010 node manager, where the attach ordering is often not theirs to choose. Two tests that **could not fail** were also replaced: one asserted `AccessLevel` equals `CurrentRead` to pin a read-only marking, but `BaseVariableState` initialises every variable to exactly that, so deleting the marking left it green; it is now a write expecting `BadNotWritable`, including a real OPC UA `Write` service call over a live session. Finally, a build break that a plain build hides: `RoboticsAotTests` used `out string?` in a project declaring `<Nullable>disable</Nullable>`. Building the project **without** `-f` suppresses `CS8632` through `NoWarn=nullable` and succeeds; building it **with** `-f net10.0` resolves `NoWarn` differently, `CS8632` fires, and `TreatWarningsAsErrors` makes it an error. CI builds per-TFM, so this would have failed there while passing every local build to that point. ## Testing and coverage | | | |---|---| | `dotnet build UA.slnx -t:Rebuild` | **0 errors, 2 warnings** — both pre-existing `CA1861` in `tests/Opc.Ua.Tools.Tests`, a file this PR does not touch | | **Whole `UA.slnx` test suite, net10.0** | **50 of 52 assemblies fully green** (measured at `40d9cf213`, before the `master` merge; the other two are explained below) | | `Opc.Ua.Robotics.Tests` | **467** net10.0, **447** net48 | | `Opc.Ua.Robotics.Intent.Tests` (live client <-> server) | **20** on net10.0 and net48, run repeatedly | | `Opc.Ua.OpenUsd.Tests` | **904** after merging `master` (664 before; the rest arrive with #4160 / #4162 / #4180) | | `Opc.Ua.Aot.Tests` | **121/121** | | `Opc.Ua.Di.Tests` (regression from the sample move) | **364/365**, one pre-existing skip | | **Changed-line coverage** | **90.7 %**, no file below 80 % — comfortably above the enforced band, which after #4158 applies only to patches larger than 100 changed lines | The two non-green assemblies are **not** caused by this branch. Rather than assert that, I ran the same selections at the merge-base `ae241b662` on the same machine: - `ServiceCallReassemblerTests.PrivateRequestSummaryFormatsKnownRequestKinds` expects `12.5` and gets `12,5` — a culture-sensitive assertion failing on a European locale. Fails 4/4 on **both** commits. - `CertRotationLiveTests.L2Cert2RotateCertificateDuringServerRestartRecoversSharedChannelAsync` fails on a teardown count. Fails 4/4 on **both** commits. - A third failure seen once, AOT `FindServersAsync` with `BadSecureChannelClosed`, did not reproduce: 4/4 on retest and 121/121 in the final run. Environmental. Two things are worth calling out because they would otherwise be invisible: - `tools/**` is **not** in the `coverage-thresholds.json` ignore list, and no test project referenced the OpenUSD tools assemblies — so their changed lines counted as uncovered and the real patch figure was 75 %, exactly the blocking threshold. Rather than widening the ignore list, the pure decisions in the viewport host (pointer-to-physical-pixel conversion, `RenderPickRequest` construction, stale-retry, backend discovery, pick-mode selection) were separated from the Avalonia/native shell and tested headless against a fake picking backend. - `dotnet build UA.slnx` **hides warnings on unchanged projects**; only `-t:Rebuild` surfaces them. That is how a `CA2263` introduced by a net48 portability fix survived several "zero warning" builds. The multi-target requirement earned its keep repeatedly: it caught a default interface implementation, `ArgumentNullException.ThrowIfNull`, `string.Create` with an interpolated handler, `ValueTask.FromResult`/`CompletedTask`, `Task.WaitAsync`, generic `Enum.GetValues` and generic `Enum.IsDefined`, and a non-generic `TaskCompletionSource` — none of which the net10.0 build complained about. A net10.0-**only** hang in the simulated executor and two load-sensitive flaky tests were also found and fixed at the cause. ### Merged with `master` `master` (through `e73e71184`) is merged in as `08205d81d`. Four conflicts, all where the two branches touched the same robot and OpenUSD surfaces, and none resolved by dropping a side: - #4162 added six files to `samples/MinimalRobotServer/` while this branch moved that directory to `samples/Robotics/MinimalRobotServer/`. They are kept at the moved path, and `UA.slnx` takes the union of both sides' new projects. - #4160 replaced `object` with a `UsdValue` union and rewrote `Flatten`; this branch had reformatted the same method and fixed a path in its doc comment. Git produced a hybrid that did not compile. The resolution keeps `master`'s `UsdValue` arm and `Flatten(UsdValue)` overload in this branch's block-bodied form, with the path fix retained. - `master` inserted `FindStageCamera` directly before two methods this branch had relocated, so the merge duplicated `WriteStageUsda` and `GetPrivateStateRoot`. Only `FindStageCamera` is genuinely new — `master`'s diff shows the other two as unmodified context — so the duplicates are dropped and this branch's `internal` versions kept, including the `GetPrivateStateRoot(string?)` overload the connector tests use to stay out of the real user profile. - `master` moved the OpenUSD stack to `0.4.0-alpha`; this branch pins `OpenUsd.Rendering` in the test project so the renderer-pick tests can reach the picking backend. Left at `0.1.0-alpha` that pin is below what the viewer now resolves transitively, which is `CS1705`. The pin follows `master`. CI was green on `baaf35e9` with **190 checks passing and none failing**, across every build leg (net472 / net48 / netstandard2.0 / netstandard2.1 / net8.0 / net9.0 / net10.0, Debug and Release) and the full test matrix on Windows and Linux. Since then `master` has been merged twice more and two CI-stability fixes landed (see below), so the current head is re-running. Locally, `dotnet build UA.slnx -t:Rebuild` gives **0 errors** and the 2 pre-existing `CA1861` warnings; Robotics **467** net10.0 / **447** net48, Robot Intent **20** on both, OpenUSD **904**, AOT **121/121**, source generation **3771**. ### Review round Five reviewers over disjoint areas found defects worth calling out, all fixed in `3ffec2c1`: - **A refused mission step was reported `Succeeded`.** The advance path returned the same `false` for "steps exhausted" and "step refused", so a protective stop between steps told a supervisory system the mission had completed while the robot stood halted with work unexecuted. - **Three attacker-controlled NodeIds reached the executor unvalidated** — `Pick.Tool`, `Place.Tool`, `ToolChange.DockStation`. `DockStation` drives a physical tool-exchange motion to a location the server never authorised. The replacement test enumerates NodeId members by reflection so the next one added cannot be missed. - **A lock-order inversion that loses a stop.** `CanCancel` and the mission condition evaluator ran under the host lock while the executor's own thread re-entered it via `ReportBlendBegin`. - **A non-terminal executor outcome killed the pump** from outside the `try/catch` that exists because the executor is untrusted — `Ready` stayed true and submissions kept being accepted while nothing moved. - **`Pause` published `Suspended` while the robot kept moving.** Now queue-only and honest about it; the specification gained the matching rule ([opcua-drafts `357a558`](https://github.com/marcschier/opcua-drafts/pull/47)). - **`RI-RealTimeChannel` and `RI-Safety` were false claims** — granted on an empty folder with no methods, and on the mere presence of a Mandatory node. - **The Part 10 promotions never promoted.** Namespace-1 BrowseNames declared second members beside the inherited ones; the conformance test could not see it because its lookup stripped the prefix. Fixed upstream and regenerated. - **Renderer picking never worked.** The reflective probe bound a renderer that is not a picking backend and throws off its owner thread. It now binds only `IRenderPickingBackend`, falls back immediately and loudly, and the docs say plainly that renderer picking is unavailable with this package version. - **The client lost command authority the instant it gained it** — monitored items report the current value first, and the lease treated every notification as a loss. ### CI stability fixes for master-side failures The CI split in #4158 turned on legs that had never run, and two of the failures they exposed were real bugs on `master` rather than anything this branch introduced. Both are fixed here because they were blocking every branch: - **ECC key generation was broken on macOS.** `ExportExplicitParameters` throws `PlatformNotSupportedException` there — Apple's SecurityTransforms supports named curves only — and it was called for exactly one value, the curve order `n`. The four curves OPC 10000-12 admits have published orders, so those are carried directly, with `CurveOrderTableMatchesThePlatform` checking each against the platform's own exported order wherever export works, so the constants cannot silently drift. This accounts for the two ECC failures in `test-macOS-latest-Server`. - **The certificate-leak detector raced asynchronous disposal.** It polled a fixed budget and reported whatever it saw, which cannot distinguish a genuine leak (count steady) from a suite still draining fire-and-forget disposals (count falling) — on a loaded agent that produced `Certificate leak detected: 2 instance(s)` with every test passing. It now waits for the count to **settle**, so a real leak is reported sooner than before while a slow drain gets as long as it keeps progressing. Shared by every suite that asserts no certificate leaks. The remaining macOS failures on `master` (`Gds`, `Tools`, `Redundancy.Samples`, `aot-macos`) have different root causes, are not reproducible without a macOS host, and are **not** addressed here. ### Merged with `master` again `master` through `416d619c4` (#4158, the CI matrix split and graduated coverage bands) is merged in as `08ae4528`. One conflict, in `docs/DeveloperGuide.md`, where both sides had deliberately rewritten the coverage-gate section: - `master`'s rewrite is kept in full — graduated `patch.bands`, Codecov being informational-only, and where the numbers surface per CI system. - This branch's paragraph about the `ignore` globs is kept too, re-sited ahead of those subsections. I re-checked it against the merged `coverage-thresholds.json` rather than assuming it still held: the list is still `tests/**`, `samples/**`, `obj`, `bin`, `*.g.cs`, with **`tools/**` absent** — so the warning that anything under `tools/` is measured like product code, and that an assembly no test project references contributes changed lines counted as *uncovered*, is still accurate and worth keeping. - `master`'s "reproduce a coverage failure locally" line supersedes this branch's near-identical one, so only `master`'s survives. Post-merge: `dotnet build UA.slnx -t:Rebuild` **0 errors**, 2 pre-existing `CA1861`; Robotics **467** net10.0 / **447** net48; Robot Intent **20** on both. ### Getting CI green The pipeline had never actually run on this branch until now (it was waiting on the fork-PR approval gate), so the first run surfaced four real problems, each fixed at source: - **Seven build legs failed** because `IntentEnabledRobot` pins `net10.0` for the OpenUSD payload but never opted into `RestrictForLegacyTfm`. On the legacy legs the libraries build as net472/net48, where `Opc.Ua.Types` exposes the `System.Threading.Lock` polyfill publicly, and that collides with the sample's own net10.0 BCL (`CS0433`). `Opc.Ua.OpenUsd.Connector.Viewer` was already using the opt-out for the same reason. - **A Linux-only test failure** turned out to be a real defect, not a platform quirk: `SetPose` published the *commanded* interpolated pose beside joint frames derived from the *solved* joints, so the two disagreed by the inverse-kinematics residual — about 8 mm, constant after any real motion. That is the lying-digital-twin defect one level deeper than the per-step IK fix reached. `SetPose` now derives both from the joint angles. - **Three source-generator tests** asserted the old emitted shape. The clause 6.2 Null-Variant fix was implemented in `ObjectTypeProxyGenerator` so every generated proxy benefits, which changed that shape; the tests are updated and strengthened to pin the `IsNull` branch as well. - **A CI-only timeout** in the simulated arm drain: the executor runs on a manual clock so the tests do not depend on machine speed, but the drain guarded itself with a wall-clock deadline, which put that dependence back. CI runs with coverage instrumentation, which is enough to take a per-step IK probe from ~0.6 s to over 5 s. The guard is now a simulated step budget. One failure was **not** from this PR and did not recur: a Roslyn `AccessViolationException` compiling `Opc.Ua.ISA95` on one leg, while the Debug leg of the same target passed on identical code and the Release build succeeds locally. ## Related Issues - Relates to #3827 — the request that prompted the robotics intent work. - Builds on #4127. The companion specification is drafted in the open at [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) (`metaverse-specs/robot-intent/`), with the prior art and the reasoning behind each decision in the research document beside it. **Nothing in it is normative or endorsed by the OPC Foundation, and its NodeIds and namespace URI are provisional** — which is the main reason this is a draft. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. On the test checkbox, precisely: the whole `UA.slnx` suite now runs on net10.0 with **50 of 52 assemblies fully green**, and `Opc.Ua.Robotics.Tests` passes on **all five** target frameworks including net472, net8.0 and net9.0 which had never previously been exercised. The two remaining failures are reproduced identically at the merge-base and are detailed above, so I have ticked the box; if you would rather it stayed unticked until those two are green, say so and I will revert it. ## Points I would most like feedback on 1. **The namespace and NodeIds are provisional.** They come from a draft specification, not from the OPC Foundation. If this lands before the model is registered, the identifiers will move. 2. **The model lives in `Opc.Ua.Robotics`.** It is standalone on the base UA namespace and takes no dependency on OPC 40010, so a separate `Opc.Ua.RobotIntent` package would arguably be cleaner. Keeping it here was a deliberate call — this is where the robot-facing API already is — but it is the easiest thing to change now and the hardest later. 3. **Removing the convention API from #4127 is a breaking change** for anyone who adopted it. It was explicitly marked non-normative, so I have assumed that is acceptable — please say if a deprecation period is wanted instead. 4. **Serial execution** is a deliberate simplification. Concurrent `None`/`Soft` intents would be an optimisation, not a correction, but it is worth agreeing that reading. 5. **`samples/Robotics/` reorganisation** moves `MinimalRobotServer`. Happy to split that into its own commit if it makes review easier. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dab1302-19c5-4a9b-a50c-d97d389713aa Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b	2026-08-08 09:08:40 +02:00
Marc Schier	fb80bfdc82	[WotCon] Track WoT Connectivity/Binding 1.1 and add projections (Section 12) (#4182) ... Follow-up to #4128, which merged the WoT Connectivity 1.1 work against **draft2** of the companion specifications. Both drafts have moved since, and this branch tracks them through to **draft3**, adds the projection feature they introduced, and closes the gaps a bidirectional re-audit found. The specifications are the private [`OPCF-Members/spec-drafts`](https://github.com/OPCF-Members/spec-drafts) drafts of *OPC UA — WoT Connectivity 1.1* and *OPC UA — WoT Binding 1.1*. They are still drafts, which is why this is opened as a draft PR — see [Status](#status) at the bottom. ## What changed in the specifications Two revisions landed upstream while #4128 was in flight. **draft2** replaced the `eventGroup` / `actionGroup` grouping constructs with a single **projection / view** concept, added fifteen Section 6 vocabulary terms, and made two identifier rules normative. **draft3** ([spec PR #8](https://github.com/OPCF-Members/spec-drafts/pull/8)) un-deprecated the incorporated OPC 10100-1 v1.02 surface, restructured the conformance clause into three profiles, and tightened two security obligations. The draft3 NodeSet is **structurally identical** to draft2 — I diffed all 286 nodes node-by-node. Every change is metadata: 96 `ReleaseStatus="Deprecated"` attributes removed, `PublicationDate` advanced, and `WOTC-ProjectionMaterialization` claimed by three Type Nodes that previously implemented it in name only. ## Projections (WoT Binding Section 12) The substantial addition. A projection document selects affordances from other Thing Descriptions or Thing Models and resolves to a document of its own; materialized, it becomes an OPC UA `View` that **creates no affordance Node** and only `Organizes` what its sources already materialized. | Piece | Where | |---|---| | Document model and validation | `Opc.Ua.Types/Wot/WotProjection.cs`, `WotProjectionModel.cs` | | Four-stage resolution engine | `Opc.Ua.Types/Wot/WotProjectionResolver.cs` | | Materialization plan | `Opc.Ua.WotCon.Server/Materialization/WotViewProjectionPlan.cs`, `WotProjectionViewBuilder.cs` | | Live address-space host | `WotProjectionViewNodeManager.cs`, `LifecycleWotViewProjectionHost.cs`, `IWotViewProjectionHost.cs` | | Node lookup seam | `IWotMaterializedNodeIndex.cs` | Groups reached by `ua:Organizes` become organizational Objects rather than nested Views, only the outermost materialization is a `View`, and a member whose source is served by another server is omitted and reported rather than recreated — the federation case the clause calls out. ### How this is validated The specification publishes twenty worked examples, two of which are a **golden pair**: a projection document and the resolved view it is defined to produce. `WotSpecExampleTests` embeds all twenty and runs the pair through the resolver, asserting against *the specification's own expected output* rather than against my reading of the prose. That single document exercises all three selection forms, the bulk naming rule, the security closure naming and the provenance term. This is the strongest evidence in the PR, and it is worth more than the unit tests around it: it is the same check the upstream `validate_local.py` performs. ## Other spec work - **Section 6 vocabulary** — fifteen missing model and platform terms with their validation rules (`WotNodeSetConverter.ModelVocabulary.cs`). - **Portable identifiers** — the two rules release 1.1 made normative, with `WotNodeSetConverterOptions.AllowNonPortableIdentifiers` as a documented downgrade-to-warning switch for documents authored against v1.00. Defaults to `false`. - **xRegistry §6.9 identifier construction** (`XRegistryIdentifier.cs`) — reverse-authority symbolic identifiers; all seven published worked examples are test cases. ## Gaps found by re-auditing against draft3 I audited in both directions — every normative requirement to code, and every `uav:` term in code back to a spec that defines it. Three findings, all fixed: 1. **`Wot-Con 1.02` accepted anything well-formed.** draft3 makes format validation part of the unit, because *WoT-Con Minimal* is one unit wide and materialization projects a *valid* document. The 1.02 upload path only deserialized into the `ThingDescription` POCO, which happily accepts JSON that is not a WoT document at all — every member of that type is optional, so `{}` deserialized and was materialized. It now parses via `WotDocument.Parse` and requires the document to identify itself by a non-empty `name` or `title`, returning `Bad_DecodingError` and materializing nothing otherwise. The rule lives in `ThingDescriptionFormatValidator` and is shared with `CreateAssetForEndpoint`, whose auto-generated Thing Descriptions §11 also treats as untrusted input. 2. **Two `WoTFile` operations were ungated.** draft3 §11 names `Write` and `CloseAndUpdate` alongside the five management methods; both reached the materializer without an access check. `EnforceManagementAccess` is now wired into `WotAssetFileManager`. 3. **Two invented `uav:` terms.** `uav:NodeSet2Preservation` was declared and never used — removed. `uav:eventFields` is read by `OpcUaBindingPlanner` and defined in *neither* spec, while a comment claimed it came from OPC 10101. It still works for compatibility but is now documented as a non-standard extension at both sites rather than misattributed. The SSRF prohibition is satisfied by construction: there is no HTTP client anywhere in these assemblies, and all resolution goes through the injected `IWotThingResolver`. ## Two ambiguities fixed in the specifications themselves The audit turned up two places where a conforming implementer had to invent an answer. Rather than pick silently, I took both upstream — they are pushed to [spec-drafts PR #8](https://github.com/OPCF-Members/spec-drafts/pull/8) (all checks green) and are now normative in *Binding* §12.6 and *Connectivity* §7.13: - **`ViewVersion`** had one sentence saying only *that* it changes. It now changes iff the resolved membership changes, is updated at commit time alongside the `NodeVersion` stamping, and is a deterministic function of the membership alone in a canonical order — so two servers that resolved the same membership agree, which a per-server counter would not across a redundant pair. Explicitly **not monotonic**: compare for inequality only. - **"Type-level view"** was used once in §12.2 and defined nowhere, while every other statement of materialization was universal. §12.6 now *defines* it rather than contradicting §12.2: the NodeClass is `View` in both cases. Both were already what this code did, so neither changed behaviour — but the order-insensitivity the clause added was **not** covered by any test, and `ViewVersionIsUnchangedWhenOnlyTheOrderOfTheMembershipChanges` now holds it. It is mutation-verified: removing the ordinal sort turns it red. `docs/WotBindings.md` no longer lists these as points "the specification leaves open". ## Validation | Suite | Result | |---|---| | `Opc.Ua.Types.Tests` | 8525 ✅ (8518 on net48) | | `Opc.Ua.WotCon.Tests` | 1057 ✅ (net10.0 and net48) | | `Opc.Ua.WotCon.Bindings.Tests` | 559 ✅ | | `Opc.Ua.XRegistry.Tests` | 196 ✅ | | `Opc.Ua.Server.Tests` | 4027 ✅ | | `Opc.Ua.SourceGeneration.Core.Tests` | 3771 ✅ | | `Opc.Ua.WotCon.Samples.Tests` | 9 ✅ | The full `UA.slnx` builds with **0 errors and 0 warnings attributable to this PR** on all 6 TFMs (`TreatWarningsAsErrors` is on). `Opc.Ua.Server.Tests` and the source-generation suites are listed because this PR now also touches `MasterNodeManager` and the WoT-Con NodeSet. ## Compatibility No public API is removed or broken. Everything added is new surface using `ArrayOf<T>` / `ByteString` per the repo conventions, wired for DI with a direct-construction fallback, and the one behavioural tightening (`Wot-Con 1.02` format validation) rejects input that was never valid to begin with. One pre-existing issue is **not** addressed here because it came in with #4128 and is out of scope: `WotAssetFileManager.OnCloseAndUpdate` uses `.GetAwaiter().GetResult()`, which the repo rules ban. Worth a follow-up. ## Landed since this PR was opened - **DI upgraded to 1.05.0.** `ConnectsTo` was declared a subtype of `HierarchicalReferences`, contradicting OPC 10000-100 §5.5 Table 48. That is an upstream error in every official DI NodeSet up to 1.04, corrected by the OPC Foundation in 1.05.0, so the fixture is replaced with the verbatim 1.05.0 file rather than hand-patched. A guard test pins the corrected form. This also closed a second gap: six DI types the WoT model was missing are 1.05 additions. - **The sample Pump has the type definition OPC 40223 declares for it.** `Pump1.Identification` carried DI's `FunctionalGroupType` instead of `PumpIdentificationType`. Every test, the sample documents and the README agreed with each other about it, which is why it survived. - **`WotPumpAddressSpaceComparisonTests`** compares this server against `PumpDeviceIntegrationServer`, which builds the same OPC 40223 Pump from generated NodeSets and hand-written wiring. An independent oracle rather than a restatement of the same assumptions - it is what found the defect above. - **Error containment in `MaterializeProjectionViewsAsync`.** An exception mid-loop discarded the applied View handles before they were published, leaving View Nodes in the address space that nothing could remove. - **`HasWoTProjection`** is now created; it was documented and plumbed but never wired. - **The default in-memory View host** no longer deletes the View it has just applied. - **An undeclared NodeSet alias now says so.** It previously surfaced as `IdentifierMissing`, naming neither the value nor the fact that an alias was expected. - **`ViewVersion` follows the algorithm the specification now states.** My earlier spec text was rejected on review, correctly: it named neither the function nor the canonical order it demanded, ignored that OPC 10000-3 §5.4 requires a value greater than zero, and its "when, and only when" is unsatisfiable for a 32-bit value. §12.6 now specifies the algorithm outright and this implementation follows it exactly, pinned by an independently computed test vector. Implementing it surfaced a further defect in the clause - joining members on U+000A is not injective, because a NodeId string identifier may itself contain U+000A - which is fixed in the specification by length-prefixing each member. - **An auto-generated Thing Description is treated as untrusted input.** §11 requires it; `CreateAssetForEndpoint` validated the endpoint but handed whatever the discovery provider returned straight to the materializer. The `Wot-Con 1.02` format rule now gates both paths from one place. - **Document bytes moved out of the registry snapshot** into a content-addressed store, so a commit no longer rewrites the whole corpus to edit one document. Writes stage and are promoted during the commit, which preserves both durability-first ordering and the fail-closed rule that a blob directory without a manifest means operator recovery. Restoring streaming digest verification along the way closed a real gap: content integrity had degenerated to a length comparison. - **External references now reach a NodeManager registered later.** A reference to a Node that does not exist yet is dropped rather than queued, and the dynamic-registration path had no second phase to apply them in, so registering a NodeSet before the one it points into lost the forward edge permanently. Review of that change then found a state leak in its rollback path, fixed with a red-to-green regression test. - **`WOTC-Legacy` is renamed `Wot-Con 1.02`** in the specification and here. Editorial: the NodeSet diff is `<Category>` values only. ## Status Open for review. Two things may still move: 1. **The specifications are unpublished drafts.** Anything here can change if the working group decides differently. The `ViewVersion` definition has since been settled - §12.6 specifies the algorithm and this follows it - but the same could happen to any other clause tracked here. 2. **`uav:eventFields`** is kept as a documented non-standard extension. That is a judgement call the WG may want to reverse, either by adopting the term or removing it. Two known items are recorded rather than fixed: - `Pump1` has no hierarchical parent, so it is reachable by NodeId but not by browsing down from `Objects`. The cross-NodeSet ordering defect this was originally attributed to is fixed above, and cross-manager references are now symmetric in both registration orders. The Pump itself is still blocked on a different cause found while verifying that fix: with the reference added, `DeviceSet` exists but the Pump has **zero** inverse references, so the edge is dropped at *import* - before any external-reference machinery runs. That is a separate defect in the materialization path, not the one that was fixed, and it is left recorded rather than guessed at. - `WotAssetFileManager.OnCloseAndUpdate` uses `.GetAwaiter().GetResult()`, which the repo rules ban. Pre-existing from #4128 and out of scope here. Review of the projection resolver and the materialization host is the most valuable thing to look at; the vocabulary and identifier commits are mechanical by comparison. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8	2026-08-08 09:08:23 +02:00
Marc Schier	ca89ac85a7	Add a Generators companion-spec sample and compose servers at a site level (#4180) ... # Description Adds a **Generators companion-specification sample** alongside the existing pump sample, and a **`SiteCompositionServer`** that composes several device servers into one OpenUSD scene at a supervisory level — demonstrating that servers can be aggregated without mirroring their address spaces. Three things, in dependency order: ### 1. `GeneratorServer` A self-contained server realising the draft Generators spec (`http://opcfoundation.org/UA/Generators/`) end to end: N simulated generating sets, DI + Machinery integration, a twelve-state operating state machine, four protection alarms per set, six control methods, and a per-set OpenUSD twin. The organising idea is that **load fraction is the only independent variable**. Everything else is a function of it: ``` V̇_f(x) = 3.67 + 100·x fuel rate [L/h] η(x) = P(x) / (V̇_f(x) · ρ · LHV) efficiency S = P / PF I = S / (√3·V_LL) f = N·p / 120 ``` This is not stylistic. When each measurement is an independent oscillator — as the pump sample's simulation once was — a server happily publishes a duty point no real machine could occupy, and no test can catch it because there is nothing for the values to be inconsistent *with*. Deriving them from one variable means `P = √3·V·I·PF` and `η = P/(V̇·ρ·LHV)` hold at every tick by construction. The published `DATASHEET.md`, the engineering ranges, the trip points and the simulation all read the same constants in `GeneratorDatasheet.cs`, so the document and the server cannot describe different machines. The machine is a fictitious **SimGen Systems GenX-500** (400 kW prime / 440 kW standby, 400/230 V, 50 Hz, four-pole). Figures are representative of a real industrial genset class; no vendor is named anywhere in the code or docs. ### 2. Cross-server composition in the connector `RemoteSessionFactory` already existed in the client library but the connector CLI never set it, so cross-server composition was unreachable. `--federate` wires it up: the connector opens a session to each server named by a component binding, discovers its representations and drives its bindings into the same stage. It is **opt-in** because the endpoint the connector dials comes from the server being rendered rather than from the operator, which makes honouring it a trust decision. Federation is best-effort per component — a subordinate that is down is logged and skipped, and the rest of the scene still renders — which is the only `src/` change in this PR. ### 3. `SiteCompositionServer` Owns no devices. It publishes a site stage plus one cross-server component binding per subordinate, carrying that server's `ComponentServerUri` and `ComponentEndpointUrl`. Nothing is mirrored, so there is no cache to invalidate and no second copy of the truth. ## Notes for reviewers **Everything interesting in this PR was found by running the servers, not by reading the code.** Six defects, none of which looked wrong in review: - `IsShutdown` and `SubsystemName` are *optional* on `GeneratorProtectionAlarmType`, so the generated factory does not materialise them. `CreateOrReplace` alone produces a child that exists, appears in `GetChildren` and holds the right value — but carries **no `ReferenceTypeId`**, so no browse can reach it. Every alarm was publishing its trip without saying whether the trip stops the machine or which subsystem to go to. `AddXxx(context)` first is what gives the child its `HasProperty` reference. - **The protections could never fire.** Every trip point sits outside the band the datasheet curves produce — that is what a datasheet *means* — so all four alarms, the shutdown class, `ResetFaults` and the whole `Fault` branch of the state machine were unreachable while the README documented them as observable. Overload was worse: the load clamp ceiling was set to exactly the trip point, making a strictly-greater-than comparison unsatisfiable. Faults are now injectable, and the first set — the one the hero camera frames — develops one on a slow rotation. - **A shutdown trip removes the condition that caused it**, because oil pressure and coolant temperature are only supervised while the engine turns. Each alarm went active for one tick and cleared, leaving an operator with a stopped machine and no indication of why. Shutdown-class alarms now latch until the set leaves the shutdown state. Every unit test that checked the *condition* passed throughout — the defect only appears when the trip and the supervision interact over time. - **`Synchronizing` and `Paralleled` were declared, drawn in the README and never entered.** The first set now energises a dead bus and closes onto it while every other set synchronises to a live one — both how a real plant parallels, and what makes the two states observable. - **The tick raced client method calls.** The simulation tick runs on a thread-pool thread while method calls arrive on request threads, and both transition sets and write the same nodes. A tick moving a set to `Cooldown` and a concurrent `EmergencyStop` could interleave the paired `CurrentState` / `CurrentState.Id` writes and leave a client with a state *name* from one transition and a state *node* from the other — the exact failure the paired write exists to prevent, reintroduced by the threading model. Both paths now take one gate. - Low oil pressure supervised from raw speed **tripped every set during cranking**, because pressure has not built yet. Now gated on `IsSpinning`, which is what a real start-up bypass does. Smaller fixes: shutdown trips are applied after the whole evaluation pass (stopping mid-loop made the remaining conditions read healthy, collapsing simultaneous trips to whichever came first in the table); `ResetFaults` reports its clears as events (a client learns of condition state changes only through events, so a silent clear leaves an alarm-list client showing it forever); the start counter moved into the transition so commanded starts are counted; the federated connector closes the remote session if the connector constructor throws between taking ownership and registering it; and `prepare_machinery_nodeset.py` now *asserts* the IA namespace is last before removing it rather than only claiming so in its docstring — removing any earlier entry would silently renumber every namespace after it and rebind their NodeIds. **A model limitation is left visible rather than papered over.** `GeneratorStateMachineType` declares an emergency stop only out of `Running`, `Loaded` and `Paralleled`, so the sample refuses `EmergencyStop` from `Starting` and `Warmup`. A real panel stops from anywhere. That is the specification's shape, not this sample's choice, so it is pinned by a test and named in the docs. **`Model/` vendors a reduced Machinery nodeset**, derived mechanically by whitelist. The full official nodeset does not survive the model source generator (`MODELGEN003`) and drags in IA through a single optional `Stacklight` member a generating set does not have. Deriving it by script keeps the provenance checkable; the whitelist is the only thing to edit when more types are needed. **Never hand-edit the generated output.** **The federated scene renders end to end.** Getting there surfaced three more defects, all of which produced a plausible-looking scene with nothing in it: the generator sample's plant aggregation was created under an already-registered `DeviceSet` and so was invisible to every client (it had *never* rendered geometry, standalone or federated); the connector's asset fetch ran on the primary session only, so composition referenced layers that were never downloaded; and both generator colour bindings wrote `primvars:displayColor` to prims that did not declare it, so the renderer rejected every update while the file still looked correct. Cross-server components compose under the subordinate's own root, so the site layer now *places* `/Plant` and `/Powerhouse` rather than leaving them stacked on the origin. **`OpenUsdRepresentation` is now mounted with `HasAddIn`, not `HasComponent`** (review feedback). The nodeset says so explicitly — *"Mounted with HasAddIn"* — but every server in the tree used plain `HasComponent`. It survived because `HasAddIn` is a *subtype* of `HasComponent`: the representation still browses, still aggregates and still drives a twin, so every functional test passed before the fix and after it, and only a conformance checker could tell. Seven mount sites corrected. `PumpDeviceIntegrationServer` and `MinimalRobotServer` had hand-rolled the mount instead of calling the shared `CreateRepresentation` helper, which is exactly how they drifted from it. Both sample E2E suites now assert the reference type across **every** discovered representation — a narrower check would have missed the plant-level representation that arrived later from master still carrying `HasComponent`. **Live colour is not achievable in the current OpenUSD viewer, and that is now documented rather than worked around.** Proven with a standalone probe: `primvars:displayColor` resolves as `color3f[]` from the `UsdGeomGprim` schema whatever the layer declares, and the `OpenUsd 0.4.0-alpha` managed API has no writer for that type. Bound `UsdPreviewSurface` materials are not shaded either, so geometry renders grey unless it also carries an explicit `displayColor` primvar. The **visibility** bindings carry no such caveat and are what the samples rely on to show state. Filed upstream as [openusd-dotnet#2](https://github.com/marcschier/openusd-dotnet/issues/2), [#3](https://github.com/marcschier/openusd-dotnet/issues/3) and [#4](https://github.com/marcschier/openusd-dotnet/issues/4), and linked from `docs/OpenUsd.md` so a reader can check whether they have since been fixed. ## Related Issues No tracking issue — this is additive sample and documentation work with a single small, self-contained change to `src/` (per-component error isolation and session-ownership hardening in `OpenUsdConnector.Composition.cs`). Happy to open one if maintainers would prefer it tracked. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Testing The generator fixtures live in `tests/Opc.Ua.OpenUsd.Tests/Generator` — **82 tests**, no new project. `Opc.Ua.Di.Tests` would have been the natural home next to the pump fixtures, but both device samples source-generate their own reduced `Opc.Ua.Machinery` model and expose it via `InternalsVisibleTo`, so referencing both from one assembly makes every Machinery type ambiguous (CS0433); `Opc.Ua.OpenUsd.Tests` references no other sample and already hosts `RobotAssetContractTests`. The suite runs **870** on net10.0 and net48 (784 existing + 86) and **788** on net472, where the sample does not exist and only the framework-independent asset-contract tests run — forgetting exactly that exclusion broke CI on the sibling pump PR. The tests target the things in this sample that fail *quietly*: - **Datasheet conformance** holds the model to its own claim — rated speed is `120·f/p`, rated current follows from the rating, the fuel curve reproduces the published table, and `η = P/(V̇·ρ·LHV)` reconciles at every load. Efficiency is swept across the whole simulated range to show it stays inside (0,100) and rises monotonically; a fit that goes negative somewhere in its range is a coincidence that happens to look right at the duty point, not a model. A further test reads `DATASHEET.md` and checks the document quotes the figures the server actually serves. - **Drift between the two descriptions of the state machine** — the physics' `IsLegalTransition` and the model's `GeneratorStateMap` — is checked in *both* directions. A transition the physics permits but the map lacks moves a machine without telling a client; one the map holds but the physics refuses is dead weight that looks supported. - **Reachability at run time**, not just in the declared table — the test that catches states the model declares and nothing ever enters. - **That every protection can actually fire**, which is the half of the alarm contract that "a healthy set annunciates nothing" does not cover. A **hosted end-to-end fixture** connects a real client and asserts what the address space actually exposes — the aggregation at `/Powerhouse/Generators`, its component binding, and every per-set twin. That is the only kind of test that catches the registration defect above: the node object had the right browse name, binding and asset reference, so every test that inspected objects passed while no client could see it. **Every state-machine and protection fix was checked by mutation.** Restoring the clamp ceiling, the direct `Running → Loaded` path, the old start-counter placement, or removing the alarm opt-in each makes the corresponding test fail, naming the specific defect. A test that has never been seen to fail has not been shown to work. Verified against a running plant: `Synchronizing`, `Paralleled` and `Fault` all observed on a two-set server, with `HighCoolantTemperatureAlarm` seen active on the fault subject. Method behaviour verified separately — per-set distinct `CurrentState` nodes, `EmergencyStop` → `EmergencyStopped` with `Start` then refused as `BadInvalidState`, `ResetFaults` → `Off`, `SetOperatingMode(9999)` refused with the mode unchanged, and the other sets untouched throughout. Also verified as a three-server federated stage. 0 warnings on a clean Release rebuild of both samples, the client library, the connector and the tests, across every TFM. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe608993-f3c9-4779-a6b9-9a9eabfc24ba	2026-08-07 10:48:02 +02:00
Marc Schier	e73e71184d	[WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128) ... ## Summary Adds OPC UA WoT Connectivity 1.1 to the stack, together with the Types, source generation, Server and xRegistry work it is built on. This branch is the merge point for the fourteen-PR stack that replaced integration PR #4093; every constituent PR was reviewed and approved separately and is listed below. 414 files changed, +108,936 / -3,199. Roughly 46k added lines of product code, 57k of tests, plus samples, tools and documentation. ## Types - Complete `ExtensionObject` raw-body decoding for binary, XML and JSON bodies by resolving the concrete type through the message context's encodeable factory, and simplify the `Variant` structure helpers by delegating to it (#4128). - Add `IAtomicFileReplace` as an optional `IFileSystem` capability so existing external implementations keep working, with atomic publish for `LocalFileSystem` via `File.Replace`/`File.Move` and for `VirtualFileSystem` by re-keying the in-memory entry (#4128). - Add lossless conversion between WoT documents and NodeSet2 (`Opc.Ua.Wot.WotNodeSetConverter`). A byte-exact `uav:nodeSet` envelope is preserved when requested, the structured `uav:nodes` projection is used when the readable vocabulary is incomplete, and NodeSet2 is otherwise synthesized from readable WoT terms. Unmapped JSON members survive a round trip as pointer-addressed residue (#4131). ## Source generation - Improve the NodeSet to ModelDesign conversion and the generated node state (#4132). - Generate OPC UA models directly from WoT Thing Description files (#4134). ## Server - Add live NodeManager shadow and immediate reload. A reload materializes a new generation beside the active one and switches atomically; the superseded generation is retired gracefully so existing MonitoredItems keep being served until they drain, or immediately when the caller asks for it. Includes the request admission and drain machinery that lets an orderly shutdown wait for admitted requests instead of tearing down underneath them (#4147). - Make subscription transfer between sessions transactional, so a failed transfer leaves neither session holding a partially moved subscription (#4150). ## xRegistry - Allow clients to use explicit registry roots rather than assuming a well-known location (#4130). ## WoT Connectivity - Add the WoT Connectivity 1.1 information model: a registry-first revision layered on the abstract xRegistry base model, incorporating the complete published OPC 10100-1 v1.02 surface as deprecated nodes in the same namespace so existing 1.02 clients keep working (#4139). - Add the protocol binding abstractions and planners that compile WoT forms into executable plans (#4140). - Add the HTTP, Modbus, OPC UA and MQTT binding executors (#4144). - Add the registry and its stores, including document versioning, validation, dependency resolution and refresh (#4146). - Add the registry client (#4142). - Materialize WoT Thing Descriptions into the server address space, deriving types from Thing Models and instances from Thing Descriptions (#4154). - Restore the WoT asset registry bridge so the deprecated 1.02 asset surface is backed by the registry (#4171). - Add the WoT Connectivity samples and complete the documentation (#4156), and restore the remaining coverage and docs (#4172). ## New projects `src/Opc.Ua.WotCon.Bindings`, `src/Opc.Ua.WotCon.Bindings.Mqtt`, `tests/Opc.Ua.WotCon.Bindings.Tests`, `tests/Opc.Ua.WotCon.Samples.Tests`, and the `samples/WotCon` AggregationServer, AggregationClient and FlatTagServer samples. ## Constituent pull requests Merged into this branch top-down, each independently approved: | PR | Title | | --- | --- | | #4128 | [Types] Complete ExtensionObject decoding and add an atomic file replace capability | | #4131 | [Types] Add lossless conversion between WoT documents and NodeSet2 | | #4132 | [SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state | | #4134 | [SourceGeneration] Generate OPC UA models from WoT Thing Description files | | #4147 | [Server] Add live NodeManager shadow and immediate reload | | #4150 | [Server] Make subscription transfer between sessions transactional | | #4130 | [XRegistry] Allow clients to use explicit registry roots | | #4139 | [WotCon] Add the WoT Connectivity 1.1 information model | | #4140 | [WotCon] Add the WoT protocol binding abstractions and planners | | #4144 | [WotCon] Add the HTTP, Modbus, OPC UA and MQTT binding executors | | #4146 | [WotCon] Add the WoT Connectivity registry and its stores | | #4142 | [WotCon] Add the WoT Connectivity registry client | | #4154 | [WotCon] Materialize WoT Thing Descriptions into the server address space | | #4156 | [WotCon] Add the WoT Connectivity samples and complete the documentation | | #4171 | Restore WoT asset registry bridge | | #4172 | Restore WoT coverage and docs | Supersedes #4093. ## Follow-up The WoT Connectivity and WoT Binding drafts have moved to 1.1-draft2 since this work was authored. Aligning with that revision - the removed group vocabulary, the new projection/View construct, the remaining model vocabulary terms and the two new portable-identity validation rules - is tracked separately and will follow in its own pull request. ## Validation Built for every target framework with zero warnings and zero errors, and validated at the tip of the stack with `Opc.Ua.WotCon.Tests` at 999 passed / 0 failed and `Opc.Ua.Server.Tests` at 4024 passed / 0 failed. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8	2026-08-05 10:14:15 +02:00
Marc Schier	00664404c0	Ship the OPC Foundation license file from every package and fix the NU5033 pack failure (#4174) ... # Description The `preview-pack` stage of the Azure DevOps pipeline fails with nine `NU5033` errors, which fails the whole build: ``` NuGet.Build.Tasks.Pack.targets(222,5): error NU5033: Invalid metadata. Cannot specify both a PackageLicenseExpression and a PackageLicenseFile. [src\Opc.Ua.Robotics\Opc.Ua.Robotics.csproj] ``` `common.props` gives every package the shared `LICENSE.txt` as its `<PackageLicenseFile>`, but **19** packages also declared `<PackageLicenseExpression>MIT</PackageLicenseExpression>`. Nine of them hit `NU5033`; the other ten only avoided it by carrying an empty `<PackageLicenseFile></PackageLicenseFile>` workaround. That declaration was wrong on two counts. **It advertised the wrong license.** `LICENSE.txt` is the **OPC Foundation MIT License 1.00**, which carries the OPC Foundation copyright and preamble — it is a different text than the plain SPDX `MIT` license. The affected packages therefore published `<license type="expression">MIT</license>` plus a `licenses.nuget.org/MIT` URL on nuget.org, instead of the OPC Foundation text that every other package in the stack ships. **It also broke the build,** because NuGet refuses to pack a project carrying both a license expression and a license file, failing `pack` solution-wide rather than just for the offending projects. ## Changes The expression is dropped from all 19 packages so every one uses the shared OPC Foundation license file uniformly, like the rest of the stack: * `src/Opc.Ua.OpenUsd`, `.Client`, `.Server` * `src/Opc.Ua.OpenUsdScene`, `.Server` * `src/Opc.Ua.Robotics`, `.Client`, `.Server` * `src/Opc.Ua.Positioning`, `.Client`, `.Server` * `src/Opc.Ua.Di`, `src/Opc.Ua.ISA95`, `src/Opc.Ua.WotCon`, `src/Opc.Ua.Gds.Common` * `tools/Opc.Ua.OpenUsd.Connector.Viewer`, `tools/Opc.Ua.SourceGeneration.Core` * `tools/Opc.Ua.SourceGeneration.Pack`, `tools/Opc.Ua.SourceGeneration.Stack.Pack` — these two set the license not in their own `.csproj` but in the shared `tools/SourceGeneratorPack.targets`, so they were only caught once the new guard ran in CI. The eight per-project empty `<PackageLicenseFile></PackageLicenseFile>` workarounds go away with it. To stop the whole class of failure from recurring, `Directory.Build.targets` now *enforces* the rule rather than papering over it. Silently clearing the inherited license file would have fixed the build while still publishing the wrong license metadata, so instead a violation fails fast with an actionable error naming the project: ``` error OPCUA0001: Opc.Ua.Foo sets PackageLicenseExpression=MIT. Every package must ship the OPC Foundation MIT License 1.00 via the shared LICENSE.txt that common.props sets as PackageLicenseFile, which the SPDX expression neither matches nor is allowed to coexist with (NU5033). Remove the PackageLicenseExpression property. ``` `common.props` cannot catch this itself: it is imported from `Directory.Build.props`, i.e. **before** the project body, so it never observes a project-level license expression. `Directory.Build.targets` is imported **after** the project body and is the earliest place that can. The `common.props` comment that previously hinted the expression was an option now points at this rule. ### Verification * Every packable project evaluates to `PackageLicenseFile=LICENSE.txt` with an empty `PackageLicenseExpression`; file-licensed packages such as `Opc.Ua.Client` are unchanged. * `dotnet pack -c Release` succeeds for `Opc.Ua.Robotics` (previously failing) and `Opc.Ua.SourceGeneration.Core` (previously relying on the removed workaround). * Replicated the CI build commands locally — `dotnet build` of `UA.slnx` (net48/Debug), `tools/SourceGeneration.slnx` (net472/Release) and `tools/MigrationAnalyzer.slnx` (net48/Debug) all succeed with 0 errors. * Inspected the produced `.nupkg`: now `<license type="file">LICENSE.txt</license>` with the license text at the package root, matching the rest of the stack (it was `<license type="expression">MIT</license>` before). * Confirmed the guard fires on a simulated violation (`-p:PackageLicenseExpression=MIT`) and is inert otherwise. ## Related Issues No tracking issue — this fixes the `NU5033` failure in the OPC Foundation Azure DevOps pipeline (build 16247, `preview-pack` stage) and corrects the license metadata published for the affected packages. No product code or public API changes. ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. _(Build-infrastructure change with no code surface. Validated by `dotnet pack` of the affected projects, by inspecting the resulting package metadata, and by confirming the new `OPCUA0001` guard fires on a simulated violation. The pipeline `preview-pack` stage is the regression test.)_ - [x] I have added all necessary documentation. _(The rule is documented in place, on the new `Directory.Build.targets` guard and at the `PackageLicenseFile` definition in `common.props`.)_ - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received.	2026-08-04 18:49:50 +00:00
Marc Schier	81d726b7ad	Coordinate the robot cell, and make the twin show what it is actually doing (#4162) ... # Description `MinimalRobotServer` drove its two mobile manipulators through each other. The platforms *did* clear — R1 and R2 trace figure-eights 1.2 m apart at closest approach, exactly as `README.md` claimed — but each **arm reaches 1.611 m** (link offsets `0.260 + 0.680 + 0.670 + 0.158` from `robot.usda`), so the arms swept straight through one another while the chassis politely missed. Clearance has to be argued about the deployed arm, not the footprint. Three related gaps sat alongside it: motion was open-loop (two independent sine paths with no awareness of each other), only R1 had a gripper, and nothing was ever actually picked — `PartA`/`PartB` were static children of a single table and the arm replayed a canned eight-pose loop unrelated to where either the robot or the table was. This replaces all of that with a coordinated transfer cycle. ## Keeping them apart The cell is divided into reservable zones, and a robot reserves the zone *immediately ahead* before entering it, releasing the one behind once clear — block signalling rather than a global schedule: | Zone | Extent | Occupancy | | --- | --- | --- | | `EndZoneA` / `EndZoneB` | \|x\| > 1.3 m | one robot — a deployed arm needs the whole end | | `CorridorEastbound` | \|x\| ≤ 1.3 m, northern lane (y = 0.35) | one robot | | `CorridorWestbound` | \|x\| ≤ 1.3 m, southern lane (y = −1.15) | one robot | | `DockA` / `DockB` | around each charging dock (y = −2.0) | one robot | Three rules follow: - **Opposite-direction traffic passes**, being in lanes 1.5 m apart; same-direction traffic queues rather than overtaking. - **An arm may only leave its transport envelope inside an exclusively reserved end zone.** This is the rule that actually fixes the reported defect. - **A robot with no work parks on its dock**, releasing the end zone so the other robot can deliver into the station it serves. That last rule is not cosmetic. Without it the cell deadlocks permanently on the first handover: an idle robot standing beside its own table blocks the very delivery it is waiting for. A state trace showed R1 blocked at `x = 0.98` forever while R2 sat idle holding `EndZoneB`. The docks are in a southern layby rather than beside the stations, because a robot parked next to a station is close enough to foul one turning into it. ## Where the part actually is A carried part's pose is computed by forward kinematics (`RobotKinematics`) from **the same six axis values the server publishes**, not from the choreography that produced them — if the arm and the part were computed independently they would visibly drift apart. `RobotArmSolver` solves shoulder and elbow so the tool points down onto a slot, and is checked against that same forward kinematics, landing the tool on every slot of both tables to within 1e-6 m. ## Also simulated Trapezoidal accel/cruise/decel travel with the heading following the path; speed-and-separation slowdown near the other robot; an emergency stop that now halts motion rather than only blinking the beacon; battery drain with a charging detour; seeded grip-slip faults with floor recovery; and KPIs (parts moved, cycle count, last/average cycle time, utilisation, fault count). Both robots now carry a gripper. ## Two hidden couplings this surfaced - `m_r1NodeId` was keyed on *"has a tool"*, so giving R2 a gripper silently moved the dynamic-tool demo and the speed-override command target onto R2. Now keyed on R1 explicitly. - `RobotPositionsDriveLiveOpenUsdSceneAsync` sampled a fixed 1.2 s window and assumed the robot was always moving. Under a duty cycle a robot legitimately stands still while gripping, dwelling or charging, so it now polls until movement is observed — the assertion's intent is that live positions reach the scene, not that the robot never stops. ## Invariants are asserted, not assumed `CellChoreographyTests` runs ten minutes of simulated time at 50 ms and checks on **every step** that: - oriented footprints never overlap (an oriented box, not a bounding circle — the circle is so pessimistic for a 1.15 × 0.60 m platform that it demands clearances no real cell would need); - the two tool centre points never meet; - a zone is never doubly occupied; - arms only deploy inside an end zone; - parts are conserved — never duplicated, lost or carried twice; - a carried part sits exactly on the tool centre point. Plus the round trip completing, KPIs accumulating, the emergency stop halting every robot, dropped parts being recovered, and the battery draining and recharging. The README documents the zoning, traffic rules and cycle, **corrects the misleading clearance claim**, and lists ten further realism extensions that were deliberately not built (ISO 10218 / TS 15066 speed-and-separation monitoring, scanner cones, tool changer, inspection station, conveyor infeed, gate interlock, torque telemetry, maintenance counters, time-sampled pose history, traffic priority). ## Making the cycle visible Running the sample end to end against the OpenUSD connector showed the robots driving the cycle and the arms reaching the slots while **nothing was picked up or moved**. The choreographer tracked part poses and gripper opening purely in memory: the address space only ever exposed the platform poses and the twelve axis angles, so a connector had nothing to subscribe to for the parts and the blocks stayed wherever the asset authored them. A `CellTwin` folder now holds a `ThreeDCartesianCoordinates` position and a `ThreeDOrientation` heading per part, plus a jaw position per robot, bound to `/Cell/Parts/PartNN` and to the gripper jaws. The variables are registered before the motion device system is built - a binding can only carry a `SourceNodeId` that already exists - and sit outside the robotics subtree, because a workpiece is not a device and the robotics builder validates every node below its root as one it allocated itself. **The poses are published as structured vectors, not as a matrix.** A viewer composes a prim's matrix from the vector translate and rotate ops; the `matrix4d` profile is explicitly left unresolved by the reference connector, which drops a binding it cannot convert *in silence*. That is the shape the first attempt took, and it failed without a word in any log. The jaws needed the asset restructured. A carrier cube and a finger mesh were siblings under the gripper body, so driving the carrier would have slid it out from under the finger it is bolted to. Each jaw is now one `Xform` owning both, declaring the single matrix op every live-bound prim declares, with the finger points rebased onto the jaw origin. Closed puts the finger faces on the 35 mm half-width of the block, so the gripper grips it rather than passing through it. `RobotAssetContractTests` now holds **every** live-bound prim to the single-matrix contract, not just the two robot mount points. ## No part starves Collecting the lowest free slot meant a part that never landed in slot 0 was never collected at all: with two parts seeded on `WorkTableA` the second sat untouched for the life of the process while the robots shuttled the other one past it - which reads in the twin as the robots ignoring a block sitting in front of them. Parts are now stamped with the order in which they came to rest and the one that has waited longest is collected, which is what a real cell does with a buffer anyway. Verified end to end: all three parts circulate between both tables, the heading follows the carrying robot and resets on placement, and the jaws stroke between open (0.060) and closed (0.047) as parts are gripped and released. The OpenUSD packages move to 0.3.0-alpha. ## The part was not in the gripper Running it end to end showed a workpiece floating clear of the jaws that were supposed to be holding it. The platform pose and the workpiece pose leave the server as **separate bindings published by different loops**: positioning at 5 Hz (`UpdateIntervalMilliseconds = 200`), the simulation tick at 20 Hz. A carried part rides the tool centre point, so it was being drawn against a platform up to four ticks stale. Measured against the live scene with the arm stowed — where the part-to-platform distance is fixed by construction — the part sat anywhere between **0.65 m and 0.90 m** from its platform instead of at a single distance. A quarter of a metre of drift, three part widths, in front of a 96 mm jaw opening. The platform now publishes at the tick rate, and a carried part is derived from the pose that was **actually written to the address space**, recorded at the `SetFrameValue` call site rather than where the sample is produced. Each value was individually correct the whole time; only their relationship was wrong, which is why nothing caught it. `RobotOpenUsdE2eTests` now rebuilds the tool centre point from the values authored into the scene and asserts a carried part sits on it. It reproduced the defect at 0.49 m before the fix. The tolerance is the sampling straddle of reading two independently delivered bindings — one publish period is 43 mm at cruise — leaving an order of magnitude between it and the defect. Two assertions in the positioning test wanted the robots a metre apart **in X**. They pass each other in opposite corridor lanes, where X is all but identical and the separation is entirely in Y; and before the first sample arrives both platforms sit on the zero frame and coincide exactly. They now measure separation in the plane, once both have been seen to move — this was failing intermittently on net48 already. ## The opening view Framing the bounds of an enclosed scene automatically puts the eye *inside* it: the viewport opened on a close-up of whichever robot happened to be nearest, looking through the fence. The cell now authors `/Cell/OverviewCamera` — the whole floor, both work tables, the docks and the controller cabinet, from 12 m out at 28 degrees of elevation — and the connector opens on the first camera the served root layer declares. It already had the plumbing to honour a camera path; it was simply never given one. `--camera` still overrides the choice outright, and `RobotAssetContractTests` asserts the establishing shot is ordered before `/Cell/TopDownCamera`. ## The mounted gripper was never reported `DynamicToolIsComposedAsync` was failing on both TFMs, in isolation, and a longer timeout did not help. It predates this branch — I confirmed it at `58fd5e8ca` with a clean worktree. It turned out not to be a flaky test at all. **Part 5 §9.32.2** lets only a node carrying a `NodeVersion` property trigger a `ModelChangeEvent`, and `AsyncCustomNodeManager` drops entries for nodes that lack one. Neither the mounted tool nor the robot it hangs off had a `NodeVersion`, so **every addition was filtered out**. Removal kept reporting, because a deleted node is no longer in the manager's index and so takes the *"not mine, pass it through"* branch of the same filter. That asymmetry is what made it look like flakiness. A connector starting while the tool was mounted composed it and could still deactivate it later; one starting in the six-second gap was never told the tool appeared and never composed it **at all, for the life of the process**. Which happened depended only on the phase the fixture reached the test in — near-constant, hence a consistent failure rather than an intermittent one. Measured directly before the fix: model-change events arrived only on detach, exactly 18 s apart, and every resolve returned zero components: ``` 14:00:12.738 SERVER mounted <- no event 14:00:24.752 SERVER detached 14:00:24.801 model-change event <- only this ``` After enabling model-change tracking on the robot, the prim tracks the cycle — `A` = composed active (12 s mounted), `i` = inactive (6 s detached): ``` ....AAAAAAAAAAAAAAAAAAAAAAAiiiiiiiiiiiiAAAAAAAAAAAAAAAAAAAAAAAAiiiiiiiiiiii ``` **The test now asserts a transition**, not a momentary state: the prim seen both active and inactive across more than one full cycle. Unlike the old check this cannot pass on a connector that only ever hears about one direction — which is exactly what the previous pair of tests could not distinguish, since the sibling `DynamicToolIsDeactivatedWhenDetachedAsync` was satisfied by the *initial* compose and never exercised reconciliation at all. Two robustness gaps in the connector fell out of this: a rejected model-change monitored item was reported as success, leaving every `Dynamic` component frozen at whatever it resolved at start-up; and the initial compose ran *before* the subscription existed, so a change in between was seen by neither. The first now fails loudly, the second re-resolves once after subscribing. `Opc.Ua.Di.Tests` is now **402/403 on net48** (was 3 failures) and on net10.0, across three consecutive runs of the dynamic-tool tests on each. ## Related Issues - Fixes #4161 ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. Verification: full `dotnet build UA.slnx -m:1 /p:UseSharedCompilation=false` — 0 errors, no new warnings. `Opc.Ua.Di.Tests` 399/400 on **both net10.0 and net48** (1 pre-existing skip), `Opc.Ua.OpenUsd.Tests` 635, `Opc.Ua.Robotics.Tests` 103. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485	2026-08-04 06:58:29 +00:00
Marc Schier	724ae71f8a	Make NodeId assignment during a node copy explicit (#4157) ... Fixes #4137. ## The problem `NodeState.Initialize(ISystemContext, NodeState)` copies a node. Every child it materialises through `CreateChild` gets a per-instance NodeId from `ISystemContext.NodeIdFactory`, and is then initialised from its source on the very next statement - which overwrites that NodeId. The assignment is therefore *always* discarded: it only burns identifiers, and permanently leaks them for factories that track outstanding allocations. Booting the Robotics node manager left ten reservations behind that every later allocation skipped. #4123 fixed the symptom by hiding the factory for the duration of the copy, wrapping the context in one that reports `NodeIdFactory` as `null`. The review objection was fair: a context that lies about one property is hard to reason about, and it only works because that property is allowed to be null. ## What this does States the intent instead of faking the environment. NodeId assignment is now an argument of the two members that materialise children: ```csharp public virtual BaseInstanceState? CreateChild( ISystemContext context, QualifiedName browseName, bool assignInstanceNodeIds = true); protected virtual BaseInstanceState? FindChild( ISystemContext context, QualifiedName browseName, bool createOrReplace, BaseInstanceState? replacement, bool assignInstanceNodeIds = true); ``` A copy passes `assignInstanceNodeIds: false` and every node type - generated or hand-written - sees the real context. There is no capability property, no wrapper, and no second overload to keep in sync. `MethodState`, `BaseDataVariableState` and the source generator template each collapse to a single `FindChild` override that threads the argument into its `CreateOrReplace<Child>` helpers. ## Breaking change The four argument `FindChild` and the two argument `CreateChild` virtuals are gone. - **Call sites are unaffected.** The new parameter defaults to `true`, so `CreateChild(context, browseName)` and `FindChild(context, browseName, true, null)` keep compiling and keep the 1.5.378 behaviour. - **Overrides must add the parameter.** A node manager that overrides either member gets `CS0115` until it does. The compiler points at every site; `docs/migrate/2.0.x/node-states.md` shows the before/after. This is deliberate, per review: the stack ships all base types, and types generated from a model are regenerated for 2.0 anyway. ## Also fixed `MethodState.FindChild` returned `OutputArguments` when asked for `InputArguments` with `createOrReplace: false` - a pre-existing bug in the method being restructured. Covered by `FindChildReturnsTheRequestedArgumentsProperty`. ## Validation - Full `dotnet build UA.slnx`: **0 errors, 0 warnings**. - `Opc.Ua.Types.Tests`, `Opc.Ua.SourceGeneration.Core.Tests`, `Opc.Ua.SourceGeneration.Tests`, `Opc.Ua.Di.Tests`, `Opc.Ua.Robotics.Tests` - all passing. - Each new regression test was checked to fail without its corresponding fix. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485 Copilot-Session: f45109d9-59cb-4ffd-a4f7-16e1ea449ae4	2026-08-03 20:36:32 +00:00
Marc Schier	fb160a8f39	OpenUSD: the OPC UA to OpenUSD binding, scene materialization and connector (#4126) ... **Stack 4/5** — base #4125. Part of the #4096 split; see #4096 for the overall picture. > Review after #4123, #4124 and #4125. ## What this is The generic OPC UA to OpenUSD binding, scene materialization, and the connector that drives a live stage from a running server. ## Contents **Binding (Part 1)** — `Opc.Ua.OpenUsd` and `Opc.Ua.OpenUsd.Server`. A server describes how its Variables drive USD prims, and serves the USD layers it refers to as Part 5 files with a content digest. **Scene materialization (Part 2)** — `Opc.Ua.OpenUsdScene` and `Opc.Ua.OpenUsdScene.Server`, including the USDA writer. - **Escaping fix**: string values, composition-arc references, variant set names and selections, `doc` and prim names were all emitted unescaped, so a value carrying a quote or a newline could author its own layer syntax. Every emit site now goes through the escapers. The tests assert the right property — not that the payload is absent (it legitimately survives inside the escaped literal) but that no payload becomes its own line of layer syntax. **Client** — `Opc.Ua.OpenUsd.Client` streams live values into an `IUsdSink`. `CompositeUsdSink` fans out to several sinks; the file sink interpolates between server samples so motion is continuous rather than stepped at the publishing interval. **Connector** — `tools/Opc.Ua.OpenUsd.Connector` composes a stage from a live server. The optional in-process viewport lives in a separate assembly probed at run time, so the connector still runs — and says why — when it is absent. - **Fix**: the asset cache and PKI root moved off the shared temp directory to `LocalApplicationData`. **Sample** — `PumpDeviceIntegrationServer` gains an OpenUSD representation of the pump. ## Notes for the reviewer - The viewport assembly is net10.0-only. It is what exercises the `Directory.Build.targets` change from 1/5: without it, the `net8.0` leg of the per-TFM CI build fails with `CS0433` because the `System.Threading.Lock` polyfill collides with the real type. - `UsdAssetDelivery` hashes through `SHA256.Create` rather than the static `HashData`, which does not exist on the net472/net48 targets this test project also builds for. ## Validation `dotnet build UA.slnx` — 0 errors, 0 warnings. | Suite | Result | | --- | --- | | Opc.Ua.OpenUsd.Tests | 621 / 0 | | Opc.Ua.Di.Tests | 321 / 0 | --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485	2026-08-01 07:55:58 +02:00
Marc Schier	0357380335	Source generator: typed NodeSet method signatures and referenced model accessors (#4124) ... **Stack 2/5** — base #4123. Part of the #4096 split; see #4096 for the overall picture. > Review after #4123. The diff shown against `master` will include #4123 until that merges. ## What this is Source generator only; no runtime library changes. ## Contents - **Typed NodeSet method signatures.** A method imported from a NodeSet now gets a typed signature. Methods that share a declared signature are grouped onto one generated `MethodType`, named after its owner when the plain name would collide, so two unrelated `Execute` methods no longer share a class. - **An override that keeps the signature it inherits keeps the declaration it overrides.** Pointing `MethodDeclarationNode` at the override itself made `MethodDeclarationId` self-referential. OPC 10000-4 lets a caller invoke a method with the NodeId of its declaration in the ObjectType *or any supertype*, and `MethodState.MethodDeclarationId` is what the server matches on, so `Call` stopped resolving and returned `Bad_MethodInvalid` for every alarm subtype. An override that genuinely changes its signature still becomes its own declaration. - **`StateNumbers` / `TransitionNumbers` were emitted empty for every NodeSet-sourced model**, because such children arrive as `VariableDesign` rather than `PropertyDesign` and were filtered out. Any companion model built from a NodeSet was writing `StateNumber = 0` on the wire — a Part 16 defect well beyond the models in this stack. The ModelDesign path is unchanged. - **Referenced models can contribute fluent accessors**, carried by a model dependency payload so a downstream model can extend an upstream one. ## Notes for the reviewer - `tests/Opc.Ua.SourceGeneration.Tests` is left at its `master` content here. Its new vendor-subtype test uses the Robotics NodeSet as its fixture, so it can only land with the Robotics change at the top of the stack (5/5). - `PumpInstanceNodeIdRegressionTests` gains six `InputArguments`/`OutputArguments` entries for argument-less standard condition methods. Typed method signatures materialise those properties from the `ConditionType` declaration even where the method takes none. That is a real deviation, pinned rather than hidden so it stays visible; it is worth a follow-up. ## Validation `dotnet build UA.slnx` — 0 errors. | Suite | Result | | --- | --- | | Opc.Ua.SourceGeneration.Core.Tests | 3739 / 0 | | Opc.Ua.SourceGeneration.Tests | 81 / 0 | | Opc.Ua.Di.Tests | 304 / 0 | | Opc.Ua.History.Tests | 506 / 0 | | Opc.Ua.Gds.Tests | 1095 / 0 | History and GDS are listed because they are what the method-declaration fix above is load-bearing for: without it they fail with 10 and 370 errors respectively. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485	2026-08-01 07:55:57 +02:00
Marc Schier	76d33cb139	Remove SourceGenerator.Foundations, ship analyzers under Roslyn-versioned folders for VS2022 and VS2026, and validate the packages (#4120) ... # Description The two OPC UA source generators were built on `SourceGenerator.Foundations` (SGF), whose only job was getting the generator's dependency closure loaded inside the compiler. It does so with a **process-wide, first-generator-wins `AppDomain.AssemblyResolve` hook backed by a non-concurrent `Dictionary` mutated from `AssemblyLoad` callbacks**. Concurrent compilations in a shared `VBCSCompiler` race it, which is the root cause of the intermittent clean-build failure: ``` CSC : error CS8784: Generator 'StackSourceGeneratorHoist' failed to initialize. ... Exception was of type 'FileNotFoundException' with message 'Could not load file or assembly 'SourceGenerator.Foundations.Contracts, Version=2.0.14.0, ...' ``` The repository already solves that problem deterministically — #4041 made the NuGet packages ship the full closure under `analyzers/dotnet/.../cs`, and `Directory.Build.targets` registers the same closure as `Analyzer` items for in-repo `ProjectReference` consumers. SGF was therefore pure liability and is removed. ## What changed **1. SourceGenerator.Foundations removed.** `ModelSourceGenerator` and `StackSourceGenerator` are plain `[Generator(LanguageNames.CSharp)] IIncrementalGenerator` implementations again. SGF's implicit crash handling is replaced by an explicit `SourceGenerator.Guard` that reports `MODELGEN003` / `STACKGEN003` and rethrows cancellation. Its logger sink was already dead code and is dropped. `Opc.Ua.SourceGeneration.Stack.dll` shrank from **18 MB** (embedded closure) to **29 KB**. **2. Analyzers now ship under Roslyn-versioned folders.** `analyzers/dotnet/roslyn4.14/cs` and `analyzers/dotnet/roslyn5.0/cs` instead of `analyzers/dotnet/cs`, driven by `roslyn.props` at the repository root, with new `*.Pack` projects owning the package identity. The versioned folder matters: a host older than the band **skips** the analyzer instead of loading it and failing inside generator initialization. **3. Dependencies flow automatically.** Both delivery paths discover the payload by globbing the generator's output directory, so adding a package to a generator's dependency tree requires **no** `.csproj`, `.targets`, `.nuspec` or validation-script edit. Verified by temporarily adding a `PackageReference` and watching it appear in the payload with no other file touched. ## Two Roslyn bands: VS2022 and VS2026 Analyzers ship twice, once per band. The .NET SDK loads the highest band its compiler supports and **silently skips** anything above it, so a single-band package leaves every consumer below that band with no diagnostics and no generated code. | Band | Built against | Loaded by | | --- | --- | --- | | `roslyn4.14` | `Microsoft.CodeAnalysis.CSharp 4.14.0` | Visual Studio 2022 17.14+ / .NET 9 SDK | | `roslyn5.0` | `Microsoft.CodeAnalysis.CSharp 5.0.0` | Visual Studio 2026 18.0+ / .NET 10 SDK | Getting this wrong is invisible, which is most of why it took three attempts. The rule: > An analyzer must reference the **lowest** `System.Collections.Immutable` / `System.Reflection.Metadata` across all hosts it claims to support, and must never ship them. .NET binds a reference *upward* but never downward. Every failure mode is a compiler **warning**: | Diagnostic | Cause | | --- | --- | | `CS9057` | Built against a newer Roslyn than the host, so it is skipped entirely | | `CS8784` | Loaded, then `MissingMethodException` - a shipped S.C.I bound a second `ImmutableArray<T>` | | `CS8032` | Analyzer instance could not be created | This PR first shipped a `roslyn4.8` band that **never loaded** - confirmed against a real Roslyn 4.8 compiler: ``` warning CS8784: Generator 'ModelSourceGenerator' failed to initialize ... MissingMethodException: Method not found: 'IncrementalValueProvider`1<ImmutableArray`1<!!0>> IncrementalValueProviderExtensions.Collect(IncrementalValuesProvider`1<!!0>)' ``` That band was removed, and **this is not a regression**: master had the same defect, pinned to Roslyn 4.14 while referencing `System.Collections.Immutable 10.0.0.0`, so no 4.x host ever worked there either. The band that landed instead is `4.14`, and it is nearly free: `Microsoft.CodeAnalysis 4.14` depends on the **same S.C.I 9.0.0 as 5.0.0** and its `csc` runs on `net9.0`. Both bands therefore share `$(RoslynRuntimeVersion)`, so `Opc.Ua.Types` and `Opc.Ua.SourceGeneration.Core` are compiled **once** and serve both; only the five Roslyn-referencing assemblies get a second build, each from a 12-line project that glob-links the primary sources and overrides `RoslynApiVersion`. Going lower still - 4.8 wants S.C.I 7.x - would mean rebuilding that whole closure, `Opc.Ua.Types` included, and remains out of scope. (`System.Collections.Immutable 7.0.0` also does not resolve in this repository: `NU1603`, 8.0.0 substituted.) **Nothing else in the repository exercises the down-level payload**, and since every failure is a warning, a broken band would ship silently - exactly as the 4.8 one did. So `validate-source-generator-packages.ps1` now restores a real `Microsoft.Net.Compilers.Toolset 4.14.0`, runs its `csc.dll` over the packed `roslyn4.14` payload, and asserts **both** that none of `CS9057`/`CS8784`/`CS8032`/`CS8034` is emitted **and** that `/reportanalyzer` lists the generator as having executed - silence alone would also pass if the generator were never handed to the compiler. The migration analyzer, whose payload comes from a hand-written nuspec rather than `SourceGeneratorPack.targets`, gets the same check plus a band/assembly manifest assertion. To stop the trap recurring, `Microsoft.CodeAnalysis*`, `System.Collections.Immutable` and `System.Reflection.Metadata` are excluded from both the package payload and the in-repo analyzer closure, and asserted against in the validation script. `roslyn.props` and `docs/DeveloperGuide.md` document what adding a band requires. ## The shipped package set is unchanged Evaluating `PackageId`/`IsPackable` across every project in `preview-pack.slnx` gives the **same 47 package ids** on `master` and on this branch, and a full Release build plus `dotnet pack --no-build` produces exactly those 47. Only the owning project changed: | Package | master | this PR | | --- | --- | --- | | `…Opc.Ua.SourceGeneration` | `Opc.Ua.SourceGeneration.csproj` | `Opc.Ua.SourceGeneration.Pack.csproj` | | `…Opc.Ua.SourceGeneration.Stack` | `Opc.Ua.SourceGeneration.Stack.csproj` | `Opc.Ua.SourceGeneration.Stack.Pack.csproj` | `.azurepipelines/expected-packages.txt` pins the expected set and the validation script fails when the packed output drifts. ## Two new package checks **`build/<PackageId>.props` naming.** NuGet only auto-imports that exact name, and the file declares every `CompilerVisibleProperty` / `CompilerVisibleItemMetadata` the model generator relies on. It was previously packed as `build/OPCFoundation.Opc.Ua.SourceGeneration.props`, which does **not** match the package id — so **no NuGet consumer ever imported it**, and every `ModelSourceGenerator*` property and `AdditionalFiles` metadata was silently invisible. In-repo projects were unaffected because they import it by relative path. **An end-to-end consumer that actually drives the generator.** The pre-existing clean-consumer check only proved the analyzer *loads*. `Test-SourceGeneratingConsumer` builds a standalone project that feeds a real NodeSet2 in as an `AdditionalFile`, pins a `ModelSourceGeneratorPrefix` that cannot be derived from the model, and references the emitted types from hand-written code. It deliberately does **not** import the props by path, so the package's auto-import is what is under test. Both checks were verified to fail against a package rebuilt with the misnamed props. ## CI: parallelism re-enabled `buildandtest.yml` pinned three `dotnet build` invocations to `/p:UseSharedCompilation=false` and `-maxcpucount:1`. `dotnet build` defaults to all logical processors, so `-maxcpucount:1` actively disabled parallelism. Both flags were introduced silently in unrelated PRs (`f358078b1`, `208c601b0`) and suppress exactly the conditions the SGF race needed — now structurally gone. Compilation is ~72 % of what that workflow executes (233 of 323 job-minutes in run `29916768976`), so the queue-independent `Build` step durations are the baseline: | Measurement | Baseline | | --- | --- | | `Build` step, 26 `test-*` jobs | mean 5.53 min, sum 143.9 min | | `build-linux-all-tfm` | 56.3 min | | `build-windows-all-tfm` | 32.5 min | `codeql-analysis.yml` keeps the flag — the CodeQL tracer requires it. ## Incidental fixes - `Microsoft.CodeAnalysis.CSharp` declares an exact `[version]` range on `Microsoft.CodeAnalysis.Common`, so with `CentralPackageTransitivePinningEnabled` the central pins must move in lockstep or restore fails with `NU1109`. They now derive from `$(RoslynApiVersionVS2026)`. - `MigrationGeneratorTests` created its generator driver without parse options — silently fine under Roslyn 4.14, `Inconsistent language versions` under 5.0. - The migration analyzer props injected `<Analyzer>` items by hand from `analyzers/dotnet/cs`; NuGet registers analyzers itself and selects the matching folder, so the injection was obsolete and would now force the wrong assembly to load. - Exception diagnostics reported `ex.StackTrace` (null for an exception that never left its throw site, and carrying neither type nor inner chain) — now `ex.ToString()` at all three call sites. - The pack guard tested the flattened assembly list, so it only fired when *no* variant produced output; it now checks each declared variant contributed assemblies. - `GeneratePackageOnBuild` is disabled for the generators; packing happens in CI or on an explicit `dotnet pack`. ## Note for reviewers **Building this repository requires a Roslyn 5.x host** (the .NET 10 SDK or Visual Studio 2026). That is already the documented prerequisite and every CI leg installs `10.0.x` last, but it is now a hard requirement. ## Related Issues _No tracking issue: this is a follow-up to the CS8784 clean-build failure reported against #4041, which fixed the packaged delivery path but left the in-repo project-reference path relying on the SGF resolver._ - Follow-up to #4041 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Verification detail | Check | Result | | --- | --- | | `dotnet build UA.slnx` (clean, and after merging master) | 0 errors; 18 warnings, all pre-existing `CA1873` in `Opc.Ua.Client` | | `Opc.Ua.SourceGeneration.Tests` | 81/81 on net10.0 and net48 | | `Opc.Ua.SourceGeneration.Stack.Tests` | 92/92 on net10.0, 89/89 on net48 | | `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed, 8 skipped | | `Opc.Ua.MigrationAnalyzer.Tests` | 121/121 on net10.0 and net48 | | Package id set, master vs branch | identical, 47 packages | | Release build + `dotnet pack --no-build` | 47 packages, matching the manifest; both `roslyn4.14` and `roslyn5.0` present; no host-provided assemblies shipped | | `validate-source-generator-packages.ps1` | passes, including manifest, props-naming, end-to-end generator consumer and down-level host checks | | Down-level band under real csc 4.14.0 | all three analyzer packages load **and run** | | Negative tests | misnamed props → both checks fail as designed; hidden variant output → pack guard fires; 5.0 build placed in `roslyn4.14` → real `CS9057`; non-running generator name → execution assertion fires; expecting a `roslyn4.8` band → manifest check fires | --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9ed53262-5cfa-429a-820e-bf6146a00d8b	2026-07-31 15:46:17 +00:00
Marc Schier	8a10139b8c	Fix OPC UA address-space compliance in the device integration stack (#4117) ... # Description A live OPC UA compliance audit of `samples/PumpDeviceIntegrationServer` against the **OPC 40223 Pumps** and **OPC 10000-100 (DI)** companion specifications found **13 address-space violations** plus a serialization bug in the MCP tool. Every fix lands at its **root cause in the shared stack** rather than as a workaround in the sample, so all servers benefit. The sample now exposes **N fully simulated pumps** (`--pumps N`) and advertises the DI conformance facets it actually satisfies. > This branch has been merged with `master` including **#4116** ("Add simulation to PumpDeviceIntegrationServer and Event modelling"), which independently reworked the same pump simulation and the same fluent alarm builders. See [Relationship to #4116](#relationship-to-4116) below for how the two were reconciled. ## Findings and fixes | # | Finding | Root cause | Fix | |---|---------|-----------|-----| | 1 | `OverTempAlarm` created by the fluent API did not exist in the address space | `AttachAlarm` attached the alarm with `AddChild` but never indexed it into the node manager | Register the node; the same gap existed in `AddObject`, `CreateInstance` and the state-machine creators | | 2 | Alarms were unsubscribable — `EventNotifier = 0` everywhere, no `HasNotifier` from `Server` | No event-source wiring at all | Set `HasCondition`, initialise `SourceNode`/`SourceName`/`ConditionName`/`InputNode`, promote `EventNotifier` up the ancestor chain, publish `HasNotifier` from the `Server` object | | 3 | Every instance NodeId was minted in the **DI standard namespace** (`ns=4;s=5001_Pump #1`) | `DiNodeManager.New` inherited the parent's namespace index | New `DiNodeManager.InstanceNamespaceIndex`; instances move to the server's application namespace | | 4 | `Server/Namespaces` described only 2 of 6 namespaces | The NodeSet2 importer drops nodes parented to namespace 0, so companion-spec metadata objects never reached the address space — and no route covered the server's own namespace | New `NamespaceMetadataPublisher` walks the `NamespaceArray` and fills version/publication date from `ModelDependencyAttribute` | | 5 | Vendor BrowseNames (`Diagnostics`, `LastError`, …) sat in the DI namespace | Builders defaulted to the parent's namespace | Default to the server namespace; **spec-defined BrowseNames are unchanged** | | 6 | `AccessLevel="5"` from the Pumps NodeSet was reported as `1` | The ModelDesign `AccessLevel` enum is not `[Flags]` and cannot represent `CurrentRead \| HistoryRead` | Carry the verbatim NodeSet2 bitmask and emit named `AccessLevels` constants | | 7 | `Historizing = true` while `HistoryRead` returned `BadHistoryOperationUnsupported` | Nothing reconciled the declared history surface with the wired historians | Startup clears the advertisement (including masking the attribute read callbacks) when no provider resolves; the sample wires a real historian | | 8 | Mandatory `ProductInstanceUri` unset | Sample omission | Populated on every pump | | 9 | Mandatory `TrueState`/`FalseState` empty on every supervision boolean | Absent in the official NodeSet; a server must still populate them | Populated on every pump | | 10 | Machinery `Machines` folder empty (OPC 40001-1 §9.2 says **shall**) | `TryAddToMachinesFolder` used `AddChild`, which *re-parents* the device away from `DeviceSet` | Add the `Organizes` reference instead | | 11 | BrowseName contained a space and `#` | Sample naming | `Pump_1` with the readable label in `DisplayName` | | 12 | `Maintenance` group materialised but empty | Sample omission | Populated | | 13 | Uninitialised values reported `Good` | No initial-value status | `BadWaitingForInitialData` until the first simulation tick | | — | `ServerProfileArray` advertised only `StandardUA2017` | `DiNodeManager` did not implement the existing `IConformanceContributor` | Contribute DI 1.05 server facets computed at runtime from what is actually wired, merged with `StandardUA2017` | | — | MCP tool serialised boolean `false` and numeric `0` as JSON `null` | `Variant.Null` is `default`, so `Variant.Equals` adopts the non-null type and compares against the default value | Dispatch on `BuiltInType` and use typed `TryGetValue` accessors instead of the prohibited `AsBoxedObject` | ## Relationship to #4116 #4116 landed on `master` while this branch was in flight and targeted overlapping ground. The merge is a **union of both intents**, not a fast-forward: - **`AlarmBuilderExtensions`** — kept both sides. #4116 contributes the parent-must-be-an-Object guard, `SetEnableState`, and the `HasEventSource` references; this branch contributes the `HasCondition` reference, condition source initialisation, **registration of the alarm with the node manager so it is browsable**, notifier promotion up the whole ancestor chain, and root-notifier registration. - **`SupervisionBuilderExtensions`** — took #4116's `SetAlarmActive`. It is more spec-correct than this branch's version: it honours `EnabledState`, resets Acked/Confirmed on activation, and computes `Retain` per OPC 10000-9 so an unacknowledged Condition stays retained after going inactive. This branch's null guard was kept so a malformed alarm cannot dereference a missing `EnabledState`. - **`PumpNodeManager.Configure.cs`** — took #4116's **push-based** simulation (`RegisterPumpSimulation`, `CreatePumpSimulation`, `IValueUpdater<T>`, `Initialize`/`Advance`/`Publish`, deterministic per-pump phase offsets) as the structure, then folded in this branch's compliance work: configurable pump count, Maintenance group, `TrueState`/`FalseState`, historian wiring, per-pump mandatory Identification values, and `BadWaitingForInitialData` until the first tick. - **README** — union, keeping #4116's address-space validation workflow section. Two test adjustments were needed, both because the merge legitimately changed behaviour the tests pinned: - `PumpHostedReferenceTests` now resolves pump NodeIds from the browse result instead of hard-coding the DI namespace, and waits for the first published value. `PumpCreatedAfterStartupJoinsTheLiveSimulationAsync` was **added** to restore #4116's coverage that a pump created after startup through `ConfigureDevicesFor` joins the live simulation. - `FluentAlarmRegistrationIntegrationTests` no longer asserts that `Retain` clears when an alarm goes inactive — that pinned this branch's weaker semantics, and #4116's Part 9 behaviour is correct. ## Breaking change Runtime-created DI device instance NodeIds move from the companion-spec namespace to the server's application namespace, e.g. `ns=<DI>;s=5001_Pump #1` becomes `ns=<application>;s=5001_Pump #1`. This invalidates NodeIds cached by clients from earlier 2.0 previews. It is **not** a model change — standard DI/Machinery/Pumps type NodeIds and spec-defined BrowseNames are untouched. Device integration hosting is new in 2.0, so there is nothing to migrate from 1.5.378 and this is deliberately *not* in `docs/MigrationGuide.md`. Anyone tracking 2.0 previews should rediscover devices by browsing `DeviceSet` rather than persisting NodeIds, and resolve namespace indexes from `NamespaceArray` per connection. ## Verification Compliance is pinned by regression tests that assert **through the server's service surface** (`BrowseAsync` / `ReadAsync` / `HistoryReadAsync`), not by inspecting `NodeState` objects in memory. That distinction mattered: three defects survived in-memory assertions and were only caught through the read path — most subtly, `Historizing` was cleared while `AccessLevel` still advertised `HistoryRead`, because attribute read callbacks re-applied the NodeSet bits. Post-merge on net10.0: | Suite | Result | |---|---| | `Opc.Ua.Di.Tests` | 326 passed | | `Opc.Ua.Server.Tests` | 3846 passed, 5 skipped | | `Opc.Ua.Tools.Tests` | 288 passed | | `Opc.Ua.History.Tests` | 506 passed, 27 skipped | | `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed, 8 skipped | The full solution was also built and tested on **net48** before the merge. Review feedback from `copilot-pull-request-reviewer` has been addressed in commit `badad31dc`: the shared `ServerObjectState` lock was removed (the forward `HasNotifier` edge is published through the owning node manager instead), MCP array serialization now preserves element types, and the pump option guards report the specific rejected option. The only failure is `ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a pre-existing certificate-store issue that **passes when run in isolation** and is unrelated to this change. ## Documentation Updated `docs/DeviceIntegration.md`, `docs/MigrationGuide.md`, `docs/HistoricalAccess.md`, `docs/DeveloperGuide.md`, `docs/McpServer.md`, `docs/README.md`, `samples/PumpDeviceIntegrationServer/README.md` and `tools/Opc.Ua.Mcp/README.md`, and consolidated the four node-manager documents into a single [`docs/NodeManagers.md`](docs/NodeManagers.md) — overview, built-in managers (including `MasterNodeManager`), core vs custom, registration, server address-space metadata, and source generation — with a TOC and all inbound links repointed. ## Related Issues No tracking issue exists for this work yet — it originated from an ad-hoc compliance audit of the pump sample. Given the size of the change and the runtime-breaking NodeId move, please open (or link) a tracking issue so the design is recorded as an ADR before merging. - Fixes # ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9	2026-07-30 16:04:18 +02:00
Marc Schier	cfd15f08e1	Assign instance NodeIds in generated CreateOrReplace<Child> helpers (#4104) ... # Description The model source generator emits two families of instance helpers with inconsistent NodeId behaviour: * `ISystemContext.CreateInstanceOf<Type>(parent, browseName)` rebases the whole materialised subtree onto per-instance NodeIds. * `<Owner>.CreateOrReplace<Child>(context, replacement)` — the plumbing behind `NodeState.CreateChild` / `ReplaceChild` — did not. The child it produced was either bare (null NodeId, no browse name) or kept the type-level NodeId of the replacement. Any node manager mixing the two had to hand-roll a recursive re-stamp before registration, or sibling instances collided on NodeIds — a failure that only surfaces at registration time. `samples/PumpDeviceIntegrationServer` did exactly this. ## What changed **Generator** (`tools/Opc.Ua.SourceGeneration.Core`) * `CreateOrReplace<Child>` gains `bool assignInstanceNodeIds = true`. When enabled and the context supplies an `ISystemContext.NodeIdFactory`, a child whose NodeId is null or still equal to its declaration NodeId — and its descendants — are rebased through `NodeInstanceExtensions.AssignInstanceNodeId` / `AssignInstanceChildNodeIds`, with the owner as reference root. **A NodeId the caller already assigned is never overwritten.** * The helper also stamps the new child's `SymbolicName` / `BrowseName` / `DisplayName`. Without an identity, a path-based `INodeIdFactory` derives `{parent}_` for *every* child of a parent, so the assignment would have introduced collisions rather than removed them. `BrowseName` stamping is guarded on `context.NamespaceUris != null` — event-reporting paths pass a context without a namespace table. * All generated `Create*` factory call sites pass `assignInstanceNodeIds: false`: they build declaration subtrees whose NodeIds must stay at their type-level values, and the enclosing `CreateInstanceOf<Type>` factory rebases the finished subtree in a single pass. * The `CreateInstanceOf<Type>` rebase guard drops the `parent != null` requirement — supplying a `browseName` is what marks a dynamically materialised instance. Calling it without one (as the generated `NodeStateActivator`s do, and when replacing a well-known singleton) still keeps the declaration NodeIds. * `replacement` is now emitted as `BaseInstanceState?` on `CreateOrReplace<Child>` and on the generated `FindChild` override, matching the documented contract and the base `NodeState.FindChild` signature. **Hand-written twins** — `MethodState.CreateOrReplace{Input,Output}Arguments` and `BaseDataVariableState.CreateOrReplaceEnumStrings` gain the same parameter and behaviour, sharing a new internal `NodeInstanceExtensions.AssignNewChildInstanceNodeIds`. This is also required so the generated `public new` overrides keep hiding a matching base signature (otherwise `CS0109` with `TreatWarningsAsErrors`). **Sample** — `PumpDeviceIntegrationServer` now passes the real `DeviceSet` parent to the factory and drops `AssignChildNodeIds` plus the manual root stamp. **Docs** — new *"Materialising instances at runtime — NodeId assignment"* section in `SourceGeneratedNodeManagers.md` and a refreshed `DeviceIntegration.md`. ## Design note: why not the guard proposed in the issue Gating on *"the owner already has an instance NodeId"* cannot be implemented as `owner.NodeId != <ownerTypeNodeId>`. The generated child factories call `<Owner>.CreateOrReplace<Child>` on **declaration** instances — `CreatePumpType_Operational` runs on a `FunctionalGroupState` whose NodeId is `ObjectIds.PumpType_Operational`, not `ObjectTypeIds.FunctionalGroupType` — so the comparison would be true while the *type template* is being materialised (`forInstance: false`) and would rewrite the address space's declaration NodeIds. The explicit opt-out separates the two callers unambiguously. ## Compatibility Adding an optional parameter is source-compatible. Behaviour changes for callers of `NodeState.CreateChild` / `ReplaceChild` when the context carries a `NodeIdFactory`: the new child now carries a browse name and a per-instance NodeId instead of nothing. ## Related Issues - Fixes #4099 > [!NOTE] > The issue asks to delete `AssignChildNodeIds` from `samples/MinimalRobotServer`. That sample does not exist on `master` — it only lives on the still-open #4096 branch and carries no such helper. The equivalent boilerplate on `master` is `PumpDeviceIntegrationServer/PumpNodeManager`, which this PR cleans up. Details in [this comment](https://github.com/OPCFoundation/UA-.NETStandard/issues/4099#issuecomment-5102275507). > [!NOTE] > One commit is an unrelated one-line-scope build-hygiene fix: two `CA2000` warnings in `ConsoleReferenceClient` where subscription ownership transfers to the session. Happy to split it out if preferred. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. ### Tests added **Generator emission** (`NodeManagerGeneratorTests`) — `CreateOrReplaceChildAssignsInstanceNodeIdsByDefault`, `TypeFactoriesOptOutOfCreateOrReplaceNodeIdAssignment`, `CreateInstanceOfFactoriesRebaseWithoutAnExplicitParent`. **Helper behaviour** (`NodeInstanceExtensionsTests`) — `CreateOrReplaceArgumentsAssignsInstanceNodeIds`, `CreateOrReplaceArgumentsKeepsCallerAssignedNodeId`, `CreateOrReplaceArgumentsHonoursTheAssignmentOptOut`, `CreateOrReplaceEnumStringsAssignsInstanceNodeIds`. **Pump DI server regression** (`PumpInstanceNodeIdRegressionTests`, added on review feedback): | Test | Asserts | | --- | --- | | `ConfiguredPumpExposesTheFullModelSurface` | The exact set of **77 model identifiers** a configured pump materialises. | | `EveryPumpInstanceExposesTheSameGeneratedModelSurface` | A second pump exposes the identical generated surface, minus what the fluent `Configure` pass adds. | | `MaterialisedPumpNodeIdsComeFromTheNodeIdFactory` | Every NodeId is `{parentIdentifier}_{symbolicName}` in the parent's namespace — minted by `PumpNodeManager.New`. | | `MaterialisedPumpNodeIdsAreUniqueAcrossInstances` | No NodeId is null or shared between two instances of the type. | | `GeneratedFactoryAssignsInstanceNodeIdsBeforeRegistration` | The factory output already carries per-instance NodeIds before `AddPredefinedNodeAsync` applies its defensive rebase. | | `AlarmSubtreeKeepsStandardDeclarationNodeIds` | Pins the alarm subtree's standard-namespace ids — a pre-existing gap outside the generated helpers. | The 77-identifier baseline was captured by running the same subtree walk against the pre-change commit and diffing it against the post-change run: **identical** — same node set, same NodeIds. ### Local test runs | Suite | net10.0 | net48 | net472 | | --- | --- | --- | --- | | `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed | 3721 passed | — | | `Opc.Ua.SourceGeneration.Tests` | 81 passed | 81 passed | — | | `Opc.Ua.SourceGeneration.Stack.Tests` | 90 passed | — | — | | `Opc.Ua.Types.Tests` | 8083 passed | 8076 passed | — | | `Opc.Ua.Di.Tests` | 303 passed | 303 passed | 277 passed | | `Opc.Ua.Core.Tests` | 4134 passed | — | — | | `Opc.Ua.Gds.Tests` | 1095 passed | — | — | | `Opc.Ua.History.Tests` | 506 passed | — | — | | `Opc.Ua.Server.Tests` | 3626 passed, 1 failed | 3574 passed, 1 failed | — | The single `Opc.Ua.Server.Tests` failure — `ServerFluentApiHostingTests.ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync` — is pre-existing and unrelated; it reproduces on a stashed clean `master` tree. > [!NOTE] > The source-generation test harness compiles generated model code against a hand-written `Opc.Ua` API stub (`tests/Opc.Ua.SourceGeneration.Core.Tests/CompilerUtils.cs`, shared by three test projects). Its 73 `CreateOrReplace<Child>` declarations were updated to the new signature — without that, every generated call site passing `assignInstanceNodeIds: false` failed to bind. `NodesetMethodArgumentTests` pinned the exact emitted call and was updated to the new one for the same reason as the `NodeStateGeneratorTests` expectations. > [!NOTE] > `master` has been merged into this branch. The only conflict was in `NodeStateGeneratorTests.cs`, where master rewrapped the expected call-site strings while this branch appended `assignInstanceNodeIds: false` to them — kept master's wrapping and re-applied the argument. Master's two new templates (`Create_MethodArguments`, `Assign_MethodArgumentValues`) call `CreateOrReplace{Input,Output}Arguments` while building a declaration subtree, so they now pass `assignInstanceNodeIds: false` to preserve their stated intent of leaving the property node's identity untouched. > [!TIP] > Changing a source generator leaves dependent projects' cached generated output stale. If a local build reports `CS1739: ... does not have a parameter named 'assignInstanceNodeIds'` or `CS0109` on a generated `CreateOrReplace*`, run `dotnet build <project> -t:Rebuild`. Clean CI builds are unaffected. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b019f3a2-f58d-4f7e-93db-c8a0dec656e2	2026-07-28 21:22:14 +02:00
Marc Schier	5b9bcedc62	Add OPC UA ISA-95 Common Model and Job Control support (#4089) ... # Description Adds first-class support for the OPC UA ISA-95 companion specifications: - OPC-10030 Common Model. - OPC-10031-4 Job Control V1. - OPC-10031-4 Job Control V2. The change introduces `Opc.Ua.ISA95`, `Opc.Ua.ISA95.Client`, and `Opc.Ua.ISA95.Server`, together with a NativeAOT-compatible minimal server sample and comprehensive tests. ## Highlights - Compiles the three canonical NodeSets and identifier mapping CSV files through the source generator. - Adds NodeSet identifier mapping validation, typed method arguments/results, companion event records, and generated method argument nodes. - Provides typed Common Model builders with normative relationships and an injectable `GeoSpatialLocationType` provider. - Implements lossless V1/V2 Job Control providers over a shared version-neutral store, including state paths, status events, comments, response queries, catalog changes, and cohesive DI registration. - Adds subtype-aware typed clients, standalone encodeable registration, `IIsa95ClientFactory`, and V2 status-event streaming. - Adds the `MinimalIsa95Server` sample, package documentation, and an ISA-95 guide with conformance and HA guidance. - Tracks typed RSL/GPOS integration as follow-up issue #4095. ## Validation - `Opc.Ua.ISA95.Tests`: 137/137 passed on `net10.0` and `net48`. - Source-generator suites: 81/81 model, 90/90 stack, 3718 passed / 8 skipped core. - `EventRecordFieldReaders` feedback tests: 6/6 on `net10.0` and `net48`. - `EventRecordFieldReaders` changed-method patch coverage: 100% line coverage (36/36) and 100% branch coverage (22/22). - `Opc.Ua.ISA95`, `.Client`, and `.Server` build across all configured target frameworks without new diagnostics. - Cobertura line coverage: 88.08% model, 90.04% client, 91.19% server. - NativeAOT sample publishes and passes a live typed V2 Store/status-event validation. - The ISA-95 NativeAOT smoke test passes in the source-generated native test executable. - OPC UA KB NodeSet validation reports zero errors for Common, V1, and V2. The broader NativeAOT executable still reports unrelated existing GDS and complex-type test failures; the ISA-95-specific native test passes. ## Node state generator fixes Generating method arguments inline for imported nodesets regressed every model that already emits `InputArguments` / `OutputArguments` child node factories: - The inline block ran before the child factories. `MethodState.CreateOrReplaceInputArguments` keeps the property that was assigned first, so the child carrying the well-known NodeId was dropped and a NodeId-less placeholder remained in the address space. This broke browsing (reference descriptions without a target) and made method arguments unreachable at their well-known NodeIds. - The `MethodDeclarationId` fallback keyed on an autogenerated display name redirected every declared method of the standard address space to its own NodeId, which made method calls fail. The inline block is now emitted after the child factories and reduced to a value assignment when a child factory exists. The value is only re-emitted when an argument references a namespace other than OPC UA, because the value of an imported property is the verbatim XML of the source nodeset whose namespace indices do not survive the import. The `MethodDeclarationId` fallback is restricted to declarations without a resolvable NodeId. The generated OPC UA address space is byte-identical to `master` again. ## Related Issues - Fixes #4088 - Follow-up: #4095	2026-07-28 12:01:57 +02:00
Marc Schier	38c7356bc7	Add generic xRegistry libraries (abstract registry base model) (#4097) ... # Description xRegistry is the *abstract registry base model*: a registry-agnostic, content-addressed resource registry. It knows nothing about what a resource contains — a concrete registry supplies its own companion namespace and a fingerprinting strategy and reuses everything else. The PubSub Schema Registry in #4007 is the first specialization (its resources are schema documents), and #4093 (WoT Binding) carries a byte-identical copy of the same base NodeSet, so a shared canonical home is worth having. ## Packages | Package | Depends on | Contains | | --- | --- | --- | | `OPCFoundation.NetStandard.Opc.Ua.XRegistry` | `Opc.Ua.Core` | Abstract base companion model (compiled by the model source generator), `XRegistryWellKnown`, `IResourceContentIdProvider` | | `OPCFoundation.NetStandard.Opc.Ua.XRegistry.Client` | + `Opc.Ua.Client` | `XRegistryClient` (abstract) / `GenericXRegistryClient` (sealed), built on the generated ObjectType proxies | | `OPCFoundation.NetStandard.Opc.Ua.XRegistry.Server` | + `Opc.Ua.Server` | Fast-path / registration / federation node managers, `XRegistryServerOptions`, `IXRegistryResourceStore` | `Opc.Ua.XRegistry` deliberately depends on neither SDK, so a codec or a shared contracts assembly can reference the identity abstraction without pulling in the client or the server. ## The model is compiled, not loaded The companion model is compiled into the assemblies by the OPC UA model source generator. `Opc.Ua.XRegistry.NodeSet2.xml` is an `AdditionalFiles` generator input only — there is no `EmbeddedResource`, no runtime NodeSet parsing and no hand-written NodeId tables for anything the model declares. All three node managers load it the same way: ```csharp protected override NodeStateCollection LoadPredefinedNodes(ISystemContext context) { return new NodeStateCollection().AddOpcUaXRegistry(context); } ``` `XRegistryWellKnown` is reduced to the namespace URI plus the identifiers of the *instances* a registry materializes at runtime, which live above the model's own 63000-63999 range so they can never collide with it. ## What it does - **Content-derived identity** — `IResourceContentIdProvider` maps a document + format to a fingerprint and names the (canonicalization, hash) algorithm. Identity is derived from the bytes, not assigned by the server, so it is stable across registries — which is what makes de-duplication and federation work. - **Opaque-NodeId fast path** — a registered resource is reachable at an Opaque `NodeId` whose Identifier is the raw content-id bytes, so a consumer that received the id on the wire resolves the document in a single `Read`: no Browse, no fingerprint recomputation. The client reads it through the `ReadBytesAsync` session extension, so a document larger than `MaxByteStringLength` is fetched with range-based reads rather than failing. Optionally pre-publishes a seed resource so a fresh server can resolve one resource before any registration. - **The model's own lifecycle** — the registry root is a `RegistryType` instance serving `CreateGroup` / `GetOrCreateGroup`; each group serves `CreateResource` / `GetOrCreateResource`; and because `ResourceType : FileType`, the document *is* the file — content is streamed through the inherited `Open` / `Read` / `Write` / `Close`. `Close` fingerprints the accumulated bytes and publishes the Opaque fast-path node at runtime. Identical bytes reuse the existing node, whose lifetime is reference counted so it survives until the last resource resolving to those bytes is gone and is retired when a resource is re-written with different content. - **Optimistic concurrency** — `Delete(ExpectedEpoch)` on both resources and groups, and the same epoch check on label mutations, so a caller working from a stale read is rejected rather than clobbering a concurrent change. - **Labels** — `RegistryType`, `GroupType` and `ResourceType` each expose an `AttributesType` Object with `AddAttribute` / `RemoveAttribute`; labels are published as addressable `String` Properties. - **Federation** — a resource hosted by another registry is published as a real `ResourceType` instance whose `ExternalReference` (`ExpandedNodeId` + `ServerIndex` into the local `ServerArray`) and `ResourceUrl` carry the link and whose `Xid` carries the content-id. Because it is an ordinary `ResourceType`, a generic client drives a federated resource through exactly the same proxy as a local one. ## Extensibility A domain registry subtypes the base model — the PubSub Schema Registry declares `SchemaFileType : ResourceType` — and the generator mirrors that hierarchy across assemblies, so the proxy chain follows automatically (`SchemaFileTypeClient : ResourceTypeClient : FileTypeClient`). ```text abstract XRegistryClient ├── sealed GenericXRegistryClient // any registry namespace ├── SchemaRegistryClient (domain) └── WotRegistryClient (domain) ``` * A **domain client** derives from `XRegistryClient` and inherits the whole lifecycle. Helpers written as extension methods over a base proxy (such as `WriteDocumentAsync` on `ResourceTypeClient`) are callable on the domain proxy with no inheritance in the client layer. * A **generic client** still drives a domain registry, because a domain instance *is* an instance of the base type. This is covered by a test that defines a domain subclass over the base. ## State Document bytes live behind an injectable `IXRegistryResourceStore`. Because a resource is a `ResourceType`, which *is* a `FileType`, the store mirrors the file access model: reads and writes are **offset and length based**, so a document never has to be materialized as a whole and a ranged read touches only the bytes it needs. The document is transferred with the inherited `FileType` Methods, whose mode bits are honoured: a write that does not set `EraseExisting` starts from the stored bytes and replaces only the range it writes rather than truncating the rest. A handle is valid only on the resource and the session that opened it, and a session's handles are released when it closes. Two implementations ship: `InMemoryResourceStore` (the default, in-process) and `FileSystemResourceStore`, built on the `IFileSystem` abstraction so documents outlive the process and a shared volume can back a cluster. `XRegistryResourceStoreContractTests` is a reusable fixture that validates any implementation against the contract. ## Transport security Registry **writes always require a `SignAndEncrypt` secure channel** — a document and its content-derived identity are integrity-critical, so every mutation is rejected with `BadSecurityModeInsufficient` on a channel that is only signed or unprotected. This is not configurable. Reads are allowed on any secure channel by default; `RequireEncryptionForReads` extends the requirement to them when the documents themselves are confidential. An in-process call carries no channel and is always allowed. ## Hardening The lifecycle Methods are remotely callable, so every unbounded dimension is bounded via `XRegistryServerOptions`; exceeding a bound fails the call rather than the server: | Option | Default | Enforced on | Status code | | --- | --- | --- | --- | | `MaxConcurrentUploads` | 64 | `CreateResource`, `Open` | `BadTooManyOperations` | | `MaxResourceBytes` | 16 MiB | `Write` | `BadRequestTooLarge` | | `MaxRegisteredResources` | 4096 | `CreateResource` | `BadTooManyOperations` | ## A source generator bug fixed along the way Building on the compiled model surfaced a defect in shared generator infrastructure. `NodeSetToModelDesign.CreateDecoder` decoded values with an empty `EncodeableFactory`, so a NodeSet2's method `InputArguments` / `OutputArguments` — encoded as a list of `Argument` ExtensionObjects — never decoded, while `HasArguments` was still set. **Every** NodeSet2-sourced model in the stack was silently generating argument-less `MethodState`s and ObjectType proxies. ModelDesign-sourced models (the core stack) were unaffected, which is why this went unnoticed. Fixed in `b87726af` with a regression test that fails without it. Two defects in the xRegistry NodeSet itself were fixed at the same time: `BrowseName="1:InputArguments"` → ns-0 `InputArguments` (published NodeSets never namespace-qualify these), and the `CreateResource` / `GetOrCreateResource` **output** `VersionId` renamed to `AssignedVersionId`, which collided with the input of the same name and produced a duplicate C# parameter. ## Other fixes applied while extracting - `XRegistryRegistrationNodeManager` uses `System.Threading.Lock` instead of an `object`-typed sync root, per the repository locking rules. - Dropped the `InternalsVisibleTo` entries granting internals to `Opc.Ua.PubSub.Server.Tests`. That project only consumes public `XRegistryWellKnown` API. - The `UA.slnx` edit in #4007 accidentally **replaced** the `tests/Opc.Ua.Redundancy.Kubernetes.Tests` entry instead of adding a line, silently dropping that project from the solution. The entry is preserved here and the change is purely additive. ## Tests `tests/Opc.Ua.XRegistry.Tests` (168) covers the group and resource lifecycles, the `FileType` transfer, delete and epoch conflicts, labels, federation, both resource stores, the fast path, transport security and the extensibility contract — positive and negative paths. Passing on `net10.0` and `net48`. Patch line coverage is **98.5%**. Also adds `tests/Directory.Build.props` / `.targets`: the .NET 10 SDK prunes `Microsoft.Extensions.Logging.Abstractions` out of the test output, and coverlet's Mono.Cecil resolver only searches the output directory, so it silently fails to instrument every module that references `ILogger` and reports 0% coverage for them. These force the package back into the restore graph and copy its implementation next to the test binary. ## Documentation New [`docs/XRegistry.md`](docs/XRegistry.md), linked from the documentation index.	2026-07-28 08:45:03 +02:00
Copilot	16dd14f420	Improve MCP server coverage and agent usability (#4080) ... ## Summary - Expand deterministic MCP coverage across host startup, sessions, configuration, NodeSet export, PKI, packet diagnostics, and PubSub. - Fix Streamable HTTP startup by supplying the OPC UA client application identity. - Add client-visible required-argument errors and normalize generated tool schemas with explicit `required` arrays. - Introduce bounded `core`, `services`, `administration`, `pubsub`, and `diagnostics` tool profiles while retaining `full` as the current-major default. - Split configuration updates into focused transport, client, and security tools while retaining `SetConfiguration` in the `full` compatibility profile. - Improve tool descriptions/examples and add pinned, deterministic `mcpgrade` static/probe CI. ## Compatibility and behavior - Existing stdio clients continue to receive the `full` catalog by default. - Streamable HTTP is exposed at `/mcp`; the previous root route is intentionally removed. - `--transport http` is preferred; `--transport sse` remains a deprecated current-major alias. - Diagnostics decode/replay/key tools remain behind the existing explicit security gate. - N002 is disabled only for documented OPC UA standard and compatibility-sensitive name pairs. ## Validation - Release analyzer build: 0 warnings and 0 errors. - MCP tests: 276/276 passed with focused Coverlet collection. - MCP coverage: 82.73% lines, 78.96% branches. - MCP patch coverage: 94.79% lines (291/307 changed coverable lines). - `core`: 15 tools, mcpgrade A/100, zero findings. - `full`: 78 tools, mcpgrade A/100; only documented T001, T004, and S007 findings remain. - Full stdio probe: zero C003/C004 findings. Fixes #4079 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Copilot-Session: bb615463-338e-4bbb-9779-5bf6780b8b47	2026-07-24 20:23:04 +02:00
Jonas	142a9a40a6	Fix MCP startup failure and respect session name (#4077) ... # Description Fixes a Microsoft.Extensions.Options.OptionsValidationException when starting the MCP server and fixes the Connection tools to return and use the correct session name. ## Related Issues - Fixes #4076 ## Checklist _Put an `x` in the boxes that apply. You can complete these step by step after opening the PR._ - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [ ] I have added all necessary documentation. - [ ] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received.	2026-07-23 07:26:03 +02:00
Marc Schier	e73eb3a0e7	Fix source-generator package delivery and preview restore (Roslyn packaging) (#4041) ... ## Failure The source-generator projects disabled normal build-output packing without adding their analyzer assemblies and private runtime closure back to the NuGet payload. Debug and Release packages also shared IDs, and both preview workflows passed the unsupported `--configuration` option to `dotnet restore`. ## Fix - Package each generator and its private runtime closure under `analyzers/dotnet/cs`, excluding Roslyn host assemblies and package dependencies. - Preserve Roslyn compile references for project-reference test consumers. - Give Debug generator packages `.Debug` IDs. - Validate package contents and clean net10 consumers in both preview pipelines. - Remove invalid restore configuration arguments from both preview workflows. ## Reference - Roslyn analyzer packaging and MSBuild restore semantics; no normative OPC UA change. - Split from #4021. ## Tests - `Opc.Ua.SourceGeneration.Tests`: 68 passed on net10.0 and 68 on net48. - `Opc.Ua.SourceGeneration.Stack.Tests`: 90 passed on net10.0 and 87 on net48. - Generated `preview-pack.slnx` and restored it with `--disable-parallel`. - Packed both Release generators with `--no-build`; package-content checks and two clean net10 consumer builds passed with zero warnings/errors. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-22 12:40:27 +02:00
Marc Schier	7b5fd1f534	Preserve nullable struct null-check semantics in UA0003 (#4039) ... ## Failure UA0003 unwrapped `Nullable<T>` before classifying OPC UA built-in structs. That reported valid nullable `== null` and `!= null` checks and offered fixes whose `.IsNull` semantics differ from nullable-value absence. ## Fix - Leave `System.Nullable<T>` comparisons unchanged. - Continue reporting direct null comparisons on OPC UA structs. - Add coverage for nullable `NodeId` and `LocalizedText` operand orderings. ## Reference - OPC UA .NET 2.0 migration semantics. - Split from #4021. ## Tests - `dotnet test tests\Opc.Ua.MigrationAnalyzer.Tests\Opc.Ua.MigrationAnalyzer.Tests.csproj -c Release -f net10.0 --nologo` (121 passed) - `dotnet test tests\Opc.Ua.MigrationAnalyzer.Tests\Opc.Ua.MigrationAnalyzer.Tests.csproj -c Release -f net48 --nologo` (121 passed) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-22 04:07:45 +02:00
Marc Schier	66be6731f4	Source generator: support compiled companion models and safe NodeState writes (#4063) ... ## Summary Combines the compiled companion-model support from #4062 with the NodeState emission fixes already in this PR. The model source generator can now consume compiled companion `NodeSet2` models more reliably, while generated factories consistently target base `NodeState` members even when a generated subclass shadows them. ## Changes ### Compiled companion NodeSets - Add `OpcUaStateTypeIndex` and wire it through `ModelSourceGenerator` / `ModelCompilation` so standard (`ns=0`) node-state and method-state class names are resolved from referenced `Opc.Ua.Core(.Types)` metadata. - Add the model-generation-scoped `StandardMethodStateFallback`, which degrades an unresolved standard typed `*MethodState` reference to `global::Opc.Ua.MethodState` only when the type is neither available in the compilation nor declared by the current generation pass. - Keep declaration names outside the reference-only fallback policy, propagate fallback state through the logical execution context, reuse the immutable availability index, and use named incremental-generator inputs instead of deep `Left` / `Right` chains. - Escape generated fluent-builder XML documentation where browse names can contain XML-sensitive characters. ### Safe NodeState factory emission - Declare one base-typed `global::Opc.Ua.NodeState nodeState = state;` local per generated factory. - Use `nodeState` for base-member reads and writes to `NodeId`, `SymbolicName`, `BrowseName`, `DisplayName`, `Description`, `WriteMask`, `UserWriteMask`, `AccessRestrictions`, `ReleaseStatus`, `Categories`, `Specification` and `IsPartOfTypeHierarchy`. - Base-qualify the optional-child `previousNodeId` read, NodeId comparison, and existing-child NodeId update. - `global::`-qualify emitted `NodeId` / `ExpandedNodeId` namespace prefixes to avoid namespace collisions. ### Supporting updates - Expose `Opc.Ua.Types` internals to `Opc.Ua.Core.Encoders.Tests`. - Update generator assertions and add focused fallback regression coverage. ## Why Compiled companion models can instantiate standard types whose curated Core implementation does not expose the conventionally named typed method-state class. The scoped fallback lets those models generate without weakening the Stack generator or masking classes emitted by the current model. Separately, generated subclasses can shadow base `NodeState` properties with strongly typed `new` properties. Routing base attributes through a `NodeState`-typed local guarantees initialization reaches the intended base member while keeping generated code smaller and clearer than repeated casts. ## Validation - Full `Opc.Ua.SourceGeneration.Core.Tests` on net10.0: **3718 passed / 8 skipped / 0 failed**. - Combined focused generator coverage on net48: **21 passed / 0 failed**. - Full `Opc.Ua.SourceGeneration.Tests`: **68 passed / 0 failed**. - Full `Opc.Ua.SourceGeneration.Stack.Tests`: **90 passed / 0 failed**. ## Context This PR supersedes #4062 and contains both source-generator change sets. The companion-model fallback remains the scoped interim workaround tracked by #4060; a future upstream name-resolution step can remove it. These changes also feed the experimental data-encodings work in #4007. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-21 20:48:44 +02:00
Marc Schier	3c4661d2bf	Protect certificate and identity secrets in MCP and durable samples (OPC 10000-4 security) (#4043) ... ## Failure MCP auto-accept mutated the shared application certificate manager and accepted every certificate error, so one permissive connection could affect concurrent or later strict sessions. The durable-subscription sample also serialized complete user identity tokens, including user-name passwords and issued bearer tokens. ## Fix - Isolate auto-accept certificate managers/configurations per connection and keep their lifetime with the managed session. - Auto-accept only `BadCertificateUntrusted`; retain strict validation for all other certificate errors and shared sessions. - Version the durable store and reject the previous unsafe format. - Remove user-name passwords before persistence and reject issued-token subscriptions rather than storing bearer credentials. - Add focused tools tests plus solution, project, and durable-subscription docs. ## Reference - OPC 10000-4 Session identity-token and application-certificate security. - Split from #4021. ## Tests - `Opc.Ua.Tools.Tests` Release: 7 passed on net10.0; 4 passed on net48. - `Quickstarts.Servers` Release builds passed on net10.0 and net48 with zero warnings/errors. - `Opc.Ua.Mcp` Release net10.0 build passed with zero warnings/errors. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-07-21 11:26:25 +02:00
Marc Schier	20d9d65a64	Fix generated configuration namespace resolution (#4019) ... # Description Fix source-generated configuration types whose explicit namespace references constants emitted by the same generator pass. Roslyn cannot resolve those constants while `[DataType]` attributes are analyzed, so the generated XML codec silently fell back to `urn:memorybuffer` / `urn:testdata` while the shipped configuration uses the model namespace URIs. This change: - adds `MODELGEN021` to fail when an explicitly supplied `[DataType.Namespace]` expression cannot be resolved; - uses compile-time-available model namespace URIs for the MemoryBuffer and TestData configuration types; - restores production-shaped XML fixtures and exercises the `UpdateExtension<T>()` / `ParseExtension<T>()` path; - documents the source-generator namespace requirement and migration guidance. Targeted validation passed: - `TypeGeneratorTests`: 17 tests on `net10.0` and `net48`; - `ConfigurationRoundTripTests`: 4 tests on `net10.0` and `net48`. ## Related Issues - Fixes #4014 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received.	2026-07-18 20:15:49 +02:00
Marc Schier	84fa8e8906	CTT follow-up: monitored items, alarms, codecs, history, and model validation (#4002) ... # Description Follow-up to #3978 based on the latest OPC UA CTT aggregate, matrix, Historical Access, Attribute, and Node Management runs. ## Aggregate run 16 - Ignore synthetic `BadBoundNotFound` raw-bound markers when feeding aggregate calculators. - Add direct and live historian regression coverage. ## Multi-dimensional Variant matrices - Fix `Variant.Copy()` so rank-2 Variant matrices preserve their payload and dimensions. - Enforce valid Variant matrix dimensions consistently across Binary, JSON, XML stream, and XML parser codecs. - Encoders return `BadEncodingError`; decoders return `BadDecodingError` for malformed matrix dimensions. - Add cross-codec valid/null/empty/malformed tests and a batched ReferenceServer Read regression. - Correct the previous CTT issue attribution: the invalid `Scalar_Static_Arrays2D_Variant` server encoding poisoned the batched CTT Read. ## Node Management AddNodes - Return `BadNodeIdRejected` for unsupported requested-NodeId namespaces. - Validate hierarchical ReferenceType source/target NodeClass constraints. - Document run-18 confirmation that all remaining 15 AddNodes errors are CTT project/script configuration issues. ## Historical Access and Attribute Services run 18 - Reject whitespace in `NumericRange` syntax with `BadIndexRangeInvalid`. - Add HistoryRead tests for operation-level invalid syntax and per-DataValue `BadIndexRangeNoData`. - Document CTT defects: - `012.js` expects `BadIndexRangeNoData` at the operation level instead of on returned DataValues; - `Err-012.js` uses a non-historizing node for an access-denied test; - generic Attribute array helpers omit NodeId[] and StatusCode[] conversion paths. ## Base Information model validation run 19 - Run the CTT main session as `sysadmin` over Basic256Sha256 SignAndEncrypt so SecurityAdmin RolePermissions and EncryptionRequired restrictions are both satisfied without weakening the server model. - Preserve predefined well-known NodeIds while making generated dynamic type and optional-child factories allocate compatible per-instance NodeIds, remap internal references, and update references from the owning subtree. - Materialize dynamic RoleType properties through generated helpers and keep role-child string NodeIds disjoint from numeric RoleManager root IDs. - Add net48/net10 regressions for generator output, NodeId allocation/reference remapping, DI mandatory children, dynamic roles, and low-privilege versus SecurityAdmin Browse visibility. - Document CTT defects for permission-unaware mandatory-child validation, the UA 1.04 reference model used against a UA 1.05 server, stale ServerDiagnostics cache, and the `ConformanceUnits` scalar/array mismatch. ## Related Issues - Follow-up to #3960 and #3978. ## Checklist - [x] I have signed the CLA and read the CONTRIBUTING document. - [x] Tests prove the fixes and improve coverage. - [x] Required CTT documentation is updated. - [x] Focused builds/tests introduce no warnings. - [x] Affected tests pass on .NET Framework and .NET 10. - [ ] CI and CodeQL are green. - [ ] All PR feedback is addressed. ## Latest CTT full run and A&C run 20 - Fix Monitor Value Change V2 `036.js`: an ambiguous `DataValue(StatusCode)` call encoded `BadIndexRangeNoData` as a Good value. - Prove all 19 configured matrix monitored items notify when the selected value actually changes; document the independent `042.js` script defects. - Rebase dynamic `CertificateExpired` / `TrustListOutOfDate` alarm roots and descendants, restore standard type declarations and ModellingRules, and expose the cross-node-manager references. - Deliver method-triggered events to every authorized Session, fixing the two-Session Confirm scenario. - Preserve locale-only `LocalizedText` values consistently across Binary, JSON, and XML and align Acknowledge/Confirm comment behavior and tests. - Document the remaining proven CTT defects for TransactionDiagnostics, SemanticChange, History `013`, PolicyId scope, A&C, durable subscriptions, and Subscription Minimum events. ## Base synchronization Merged `master` through `007cfaf90` (`#4010`, repository layout consolidation) and retained the matrix regression tests at their new `tests/` locations. ## Latest validation - Focused merged-tree tests pass on net10 and net48. - `Opc.Ua.Server` and `Quickstarts.Servers` Release builds complete with 0 warnings and 0 errors on net10 and net48. - The existing `DisableEnableViaTypeAndInstanceMethodNoEventsDuringDisabledAsync` timing test took its documented inconclusive timeout path after the layout merge; no test failure was reported. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <romanett98@gmail.com>	2026-07-18 05:01:03 +02:00
Marc Schier	8538f531e7	Update paths for the consolidated repository layout	2026-07-16 06:26:05 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00