2 Commits

Author	SHA1	Message	Date
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	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