GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-21 00:56:28 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,739 Commits 110 Branches 105 Tags
fix/encoder-stack-depth-json-flake
Commit Graph

7 Commits

Author SHA1 Message Date
romanett 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) 
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

## 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.
- [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.

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-08 08:48:13 +02:00
romanett 7212607f16 OPC UA Part 3 compliance: permissions, Write semantics, NodeManagement, NodeSet DataTypes, locales (#4675) 
# Description

Fixes the High and Medium findings of an OPC UA Part 3 (OPC 10000-3,
Address Space Model) compliance audit. Each finding was checked against
the full spec text (Parts 3, 4, 5, 6, 14 and 18) before it was fixed.
Every fix comes with regression tests that fail without it.

### Role permissions (Part 3 §8.55, Part 4 §5.12.2.2)
- The type-hierarchy exemption in
`MasterNodeManager.ValidateRolePermissions` /
`ValidateAccessRestrictions` now covers only Browse, Read and
ReadRolePermissions. Type nodes stay visible to generic clients, which
was the intent of #3705; every other permission bit is now enforced on
them.
- `ReceiveEvents` is enforced on the EventType node. The standard
NodeSet grants it on `AuditEventType` and its subtypes only to
SecurityAdmin, so only SecurityAdmin sessions now receive audit events.
- Call checks the `Call` permission and AccessRestrictions on the Object
as well as on the Method.
- When a Call's `methodId` is the Method declaration of the ObjectType,
the Object's Method with the same BrowseName is permission-checked and
invoked. The declaration is used only if the Object has no such Method.
This applies to `AsyncCustomNodeManager`, `CustomNodeManager2` and the
sample node manager.

### Write service (Part 3 §5.6.2, §8.40, §8.57; Part 4 §5.11.4)
- The synchronous `OnWriteValue` path (fluent `OnWrite`) checks DataType
and ValueRank first (`Bad_TypeMismatch`). After an IndexRange write it
merges the slice into the cached value instead of replacing it.
- StatusWrite / TimestampWrite are enforced (`Bad_WriteNotSupported`).
- Undefined enumeration values and invalid OptionSet writes return
`Bad_OutOfRange`. OptionSet writes merge only the valid bits into the
stored value.
- New `ReportPropertyValueChanged` / `ReportSemanticChange` let server
code raise SemanticChangeEvents for changes it makes itself.
- Required follow-up (Part 14 §6.2.11.1): if an external target rejects
a timestamp, the PubSub subscriber's TargetVariables writer retries
without it.

### NodeManagement and Call arguments (Part 3 §5.5.2, §5.7.3, §6.4.2;
Part 4 §5.8, §5.12.2)
- AddNodes creates the Mandatory InstanceDeclarations of the
TypeDefinition and rejects abstract or Interface types
(`Bad_TypeDefinitionInvalid`).
- AddReferences, DeleteReferences and DeleteNodes bump NodeVersion and
raise GeneralModelChangeEvents.
- Clients may omit trailing optional input arguments
(`HasOptionalInputArgumentDescription`).

### NodeSet DataTypes (Part 3 §5.8.3, §8.48; Part 6 Annex F.12)
- `UANodeSet.Import` completes structure definitions with the base
type's fields (base fields first). It creates definitions for structures
without fields and sets `DefaultEncodingId`.
`StructureDefinition.FirstExplicitFieldIndex` marks the subtype's own
fields.
- `DataTypeState` no longer mutates the stored definition on read.

### Locales (Part 3 §5.2.5, §8.5; Part 4 §5.4)
- Read, Browse and data change notifications return the translation that
best matches the session's LocaleIds, or all translations for `mul`. The
selection lives in the server dispatcher and is built on
`ResourceManager`: the new `TranslateValue` calls the overridable
`Translate`.
- Locale ids are matched case-insensitively, and fallback translations
carry their real locale.

### Client complex types (Part 6 §5.1.7)
- The Reflection.Emit builder encodes only `IsOptional` subtyped fields
as ExtensionObject. It also supports OptionSet subtypes: an unsupported
type is skipped instead of aborting the whole load.
- `TypeInfo.GetXmlName` uses the data type's XML name for dynamic
structures and OptionSets (the XML body element was `<Structure>`). Both
builders now produce identical XML.

## Behaviour changes
All are documented in `docs/MigrationGuide.md`.
- **Audit events:** only SecurityAdmin receives them. The CTT "Auditing
Connections" unit needs a SecurityAdmin user (e.g. sysadmin);
`AuditingOperationTests` was updated.
- **Write timestamps and status:** clients writing a SourceTimestamp or
a non-Good StatusCode get `Bad_WriteNotSupported` unless the Variable
has StatusWrite/TimestampWrite. No variable in `src/` sets these bits.
- **AddNodes:** it rejects abstract types (including `BaseVariableType`
and `BaseEventType`), and new instances now come with child nodes.
- **Call:** the Object needs the Call permission. The Object's own
Method runs even when the call names the type declaration.

## Not included
- Low-severity findings: missing validation on optional services and
author-side NodeSet rules.
- LocalizedText nested inside structures or Variant arrays, and in
HistoryRead or Call outputs.

## Testing
net10.0, Debug, one suite at a time on the merged branch. All suites
below pass:

| Suite | Passed |
|---|---|
| Types | 11430 |
| Core.Encoders | 2947 |
| Core.Schema | 179 |
| Core.Security | 558 |
| Client | 2761 |
| Server | 8210 |
| InformationModel | 1080 |
| Features | 145 |
| History | 536 |
| Di | 455 |
| Machinery | 167 |
| Robotics | 661 |
| Robotics.Intent | 22 |
| Pumps | 75 |
| Scales | 122 |
| Positioning | 69 |
| ISA95 | 144 |
| Vision | 484 |
| PubSub | 1490 |
| PubSub.Adapter | 151 |
| PubSub.Server | 177 |
| Redundancy.Server | 856 |
| Lds | 173 |
| Configuration | 241 |
| WotCon | 1496 |

- **Client.ComplexTypes:** one test wrote a SourceTimestamp to a
variable without TimestampWrite. It is fixed in this PR, and the suite
has not been re-run since.
- **Core:** 6068 passed, 1 failure in
`ActivityTraceFlags_Metrics_ShouldRecordMetricsOnly`. That telemetry
test passed when re-run alone, so it looks flaky.
- **Still running:** Sessions, Subscriptions (Classic, Durable,
standard) and Interop. Results will follow on this PR.
- `UA.slnx` builds with 0 errors for net10.0 and net48, with no new
analyzer warnings in changed files.

## Related Issues

- Part 3 compliance audit (no tracking issue)

## 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 Opus 5.5 <noreply@anthropic.com>
 2026-10-05 12:43:59 +00:00
romanett ce88955f70 Harden encoders/decoders against untrusted input and align them with Part 6 / Part 14 (#4599) 
# Description

Hardens the encoders and decoders against untrusted input and aligns
them with OPC 10000-6 (1.05) and OPC 10000-14. This comes from an audit
of every codec reachable from a peer:
- Binary, JSON and XML decoders and encoders;
- built-in and complex types;
- UA-TCP/UASC framing;
- PubSub UADP and PubSub JSON.

The audit found 97 issues: 1 critical, 7 high, 39 medium and 50 low.
They are all fixed here, one commit per finding (IDs B1-, J1-, X1-, T1-,
S1-, U1-, P1-, E1-, E2- in the commit subjects). Every fix was then
checked against the spec a second time, and the deviations that re-check
found were fixed as well (`*-spec` commits).

## Security (decoders reachable from unauthenticated peers)

- **Allocation from length prefixes (B1-1, B1-5, U1-1, U1-3).**
- Array, string and byte-string lengths are checked against the bytes
left in the message before anything is allocated.
- Arrays read element by element grow as elements are decoded, so nested
length prefixes can no longer multiply memory.
- UADP discovery counts and chunk reassembly buffers are bounded the
same way.
- **Stack exhaustion.** Nesting is checked for remaining stack
(`RuntimeHelpers.EnsureSufficientExecutionStack` mapped to
`BadEncodingLimitsExceeded`), and the large Variant read/write methods
were split into scalar, array and matrix methods. Measured on 1 MB and
256 KB threads, no codec overflows at the default limits any more.
- **XML element depth (X1-1).** XmlElement values and raw
ExtensionObject bodies are copied node by node with a depth limit,
instead of with the recursive `InnerXml`, `ImportNode`, `InnerText` and
`DeepEquals` calls.
- **Limit bypasses:**
- JSON NodeId, QualifiedName and opaque strings now respect
MaxStringLength and MaxByteStringLength (J1-1).
  - Matrix rank is limited to 32.
  - Decimal size is bounded (T1-3).
  - XML base64 is checked before it is decoded.
  - XML arrays are checked before each element is read.
- **CPU and algorithmic cost:**
- UADP reassembly uses an O(1) overlap check with a per-message chunk
cap (U1-2).
  - PubSub JSON matches fields to metadata through a name index (P1-2).
  - LocalizedText "mul" text is parsed lazily (T1-5).
- Unknown SecurityTokenIds trigger a throttled background SKS refresh
instead of an inline call per datagram (U1-9).
- **PubSub metadata poisoning (P1-1).** Inbound metadata is registered
only for identities a configured reader expects, is capped, and an
unauthenticated message can no longer pin a version.
- **Exception contract.** Decoders fail with `ServiceResultException`
(BadDecodingError / BadEncodingLimitsExceeded) instead of letting
ArgumentException, InvalidOperationException, TypeLoadException and
similar escape.
- **UASC (S1-*):**
  - OPN and CLO must be single final chunks.
  - The certificate of an unsigned OPN is ignored.
  - The padding and ExtraPadding bytes are validated.
- A renewed token is activated only after a chunk has been verified with
it.
- A policy change on renewal is rejected before the channel certificate
is touched.
  - Secured chunk length arithmetic is bounded.

## Wire-visible behaviour changes

All follow the spec exactly; there is no reader for the old forms unless
noted.

- **UA Binary:**
  - Variant type ids 26-31 decode as ByteString.
- Picoseconds without their timestamp are ignored, and values of 10000
and above are clamped to 9999.
- A Null-type Variant with the array flag consumes its length and
dimensions.
- **UA JSON:**
- Messages are ExtensionObjects with the body inline; there is no
`UaBody` envelope any more (5.4.9 / 5.4.2.16).
- The Variant `Value` is omitted exactly for NULLs of nullable types
(Table 41).
  - RawData keeps nested UaType/UaTypeId and the StatusCode Symbol.
  - Duplicate member names are rejected.
  - `{}` decodes as a null ExtensionObject.
- Numbers and identifiers are parsed strictly and culture-independently.
  - QualifiedName follows 5.4.2.14.
  - Opaque bodies of known types carry the DataType id.
  - Enumerations without a literal are written as the number.
- **UA XML:**
  - The earliest DateTime is written as `0001-01-01T00:00:00Z`.
- Null typed scalars and null array elements are written with `xsi:nil`.
  - An ExtensionObject without a body has no `Body` element.
  - Strings XML cannot represent are rejected.
- Only the xs:dateTime and xs:date forms are accepted; a value without a
zone is still read as UTC, because published model files use it.
  - Only the Part 6 Guid form is accepted.
  - Booleans are case-sensitive in XmlDecoder.
- **UA-TCP:**
- OPNC/OPNA/CLOC/CLOA fault the channel. Older releases that split a
long-chain OPN into chunks are rejected, so this needs a release note.
  - The sender truncates the certificate chain to fit a single chunk.
  - The received certificate chain is sized from the chunk size.
- Hello buffer sizes down to 1024 bytes are honoured; below that, an ERR
with Bad_TcpInternalError is sent.
- A 4096-byte Error reason is accepted, and the SecurityPolicyUri limit
is 255 bytes.
- **PubSub UADP:**
- Chunk NetworkMessages use the Part 14 7.2.4.4.4 layout, and each chunk
is secured on its own with its own sequence number.
  - The Sizes array is part of the secured payload.
- Padded RawData keeps its length prefix, and null values are written as
-1.
  - DataSetMessages with the valid bit clear are skipped.
  - Null discovery arrays are accepted.
- **PubSub JSON:**
- Variants use `UaType`/`Value`, with the 7.2.5.4.2 top-level collapse.
The deprecated `Type`/`Body` form is still read.
- Status is written as a `{Code,Symbol}` object; a numeric Status is
still accepted.
  - Discovery messages follow Tables 188-192, with GUID MessageIds.
  - Enumeration fields are written as `Name_Value`.
  - Fields appended with a MinorVersion bump are accepted.

## Known deviations and limitations (documented, not changed)

- A known type whose ExtensionObject XML body fails to decode fails the
message, like a failing binary body. 5.2.2.15 only says the decoder
*should* continue, and continuing reopens the log-flooding path.
- Reserved Variant type ids 26-31 are passed to the application as
ByteString without the raw type id.
- JSON UaTypeId of an opaque body of an *unknown* type stays the
encoding id, because no type system is available to map it.
- PubSub JSON encoder:
- Structure fields without a subtype are still wrapped with a UaType,
and concrete multi-dimensional or abstract-rank fields still carry a
UaType (Table 186).
- Namespace-0 enumerations without EnumDataTypes are written as a
numeric string.
- Chunked transcoded frames have no writer group, so their sequence
numbers continue from the source number.
- Pre-existing and out of scope: the HTTPS JSON binding uses
Content-Type `application/opcua+uajson`, while 1.05 7.4.5 requires
`application/json`.

## Tests

- Every fix has a regression test.
- Tests that pinned non-conformant behaviour were updated, and their
commit messages say so.
- Fuzz seeds were regenerated where the wire changed.

Local results:
net10.0, 27 suites green (Types 11354, Core.Encoders 2933, Encoders.Fuzz
8112, Core 5955 + 1 known intermittent ActivityTraceFlags metrics test
that passes alone, Client 2732, Server 7802, Sessions 790, GDS 1234,
Subscriptions 641, SourceGeneration.Core 4266, PubSub 1490 and all
PubSub transport suites). Full UA.slnx build 0 warnings / 0 errors on
net10.0 and net48.

## Related Issues

- Follow-up: #4598 (possible throughput regression of per-element binary
array reads).

## 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.0
suites run; net48 build only)
- [ ] 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 Opus 5.5 <noreply@anthropic.com>
 2026-10-01 05:12:47 +00:00
Marc Schier be61d598a9 Fix startup NodeManager lifecycle and imported method arguments (#4427) 
# Description

Make startup-composed application NodeManagers participate in the live
lifecycle and bind imported NodeSet2 Method argument properties to their
typed `MethodState` children.

This change:

- adopts application NodeManagers created before startup into
`INodeManagerLifecycle.Registrations` after server startup succeeds,
while keeping diagnostics, configuration, and core managers protected;
- retains per-manager startup external-reference ownership and exact
routing positions so reload, removal, rollback, overlapping namespaces,
synchronous adapters, and server restart preserve existing behavior;
counted immutable reference snapshots avoid quadratic startup matching;
- rejects duplicate factory output across active, unpublished, and
retired lifecycle generations without deleting or disposing the live
manager; callers can catch the public
`NodeManagerAlreadyRegisteredException`;
- binds valid namespace-zero `Argument[]` `PropertyType`
`InputArguments` and `OutputArguments` Variables to
`MethodState.InputArguments` and `MethodState.OutputArguments` from
either `HasProperty` direction, without requiring `ParentNodeId`,
including local namespace-URI reference targets;
- preserves authored metadata during NodeSet typed-child promotion
without changing the initialization behavior of unrelated generated
nodes;
- retains generic-import guards for custom and malformed signatures,
duplicate standard-signature validation, and upstream factory-driven
generated-slot replacement and reconciliation callbacks;
- updates lifecycle and Runtime NodeSet documentation and clarifies
`OnServerStartedAsync` timing for both `StartAsync` overloads, including
the unopened default host returned to the caller.

This is a focused follow-up to #4418. That PR preserves registrations
from multiple fluent builders, but does not change lifecycle ownership
or NodeSet child typing. The branch includes upstream `master` through
`da7412c42`, including #4396, #4432, #4437, #4438, and #4440.

The completed review fixes in `686d0561f` are included. The latest base
merge (`a29fc6ca8`) combines them with #4432's shared batch/fluent
importer and generated state factories. One iterative linker supports
available parent nodes, deferred parent side tables, application-owned
handles, and placeholder reconciliation while retaining reference-driven
Method argument binding and URI handling. Upstream cancellation, GDS
assembly renames, certificate leak scoping, and xRegistry changes are
preserved.

## Related Issues

- Fixes #4421
- Fixes #4422

## CI regression fix

`e1a4436f0` fixes the GDS failure reported on Ubuntu and macOS. Copying
permission metadata in general `NodeState` initialization applied
`SecurityAdmin`-only defaults to generated GDS custom certificate groups
and denied their existing GDS-admin Browse requests. Metadata
preservation is now confined to NodeSet typed-child promotion; imported
Method argument permissions remain intact, while unrelated generated
nodes retain their existing initialization behavior.

Local Release validation also exposed a 260-character certificate path
in the startup-failure test added by this PR. Its isolated PKI now uses
a short temporary path. No assertions, authorization checks, or CI gates
were weakened, and no pre-existing tests were changed.

## macOS ARM64 NativeAOT suspension

Per maintainer request, `a15971f32` suspends only the failing
`macos-latest` / `osx-arm64` NativeAOT matrix entry. Re-enabling it once
the compiler contains the runtime fix is tracked in #4443. Intel macOS,
Linux, and Windows NativeAOT coverage, managed macOS tests, and all
other CI gates remain enabled. This is an explicit coverage suspension,
not a claim that the underlying compiler/linker defect is fixed.

The macOS ARM64 NativeAOT job on `a29fc6ca8` fails during native linking
with
`ld: Assertion failed: (_addend == uniqueIndex && "too many large
addends")`.
Both attempts of [run
34191755950](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34191755950)
hit the same assertion before the native test executable can run; the
build/test summary failure is downstream.

This matches the NativeAOT Mach-O relocation issue in
dotnet/runtime#119380, fixed by relocation-anchor emission in
dotnet/runtime#124721 and its .NET 10 backport dotnet/runtime#132171.
The failing `Microsoft.DotNet.ILCompiler` 10.0.11 package's nuspec
points to `dotnet/dotnet` commit
`e2f47b0110ed922f21a1522da67279133ce28f32` (July 23); its object writer
does not contain the anchor fix merged on August 12. Version 10.0.11 is
currently the latest stable package.

The base commit passed on the same runner image and SDK, so this is not
being dismissed as a generally broken runner or a test flake. The
precise PR-specific object-layout trigger has not been isolated. A
compiler containing the relocation fix is needed before restoring the
lane. No test code, assertions, coverage thresholds, or shared tooling
settings were changed.

## Validation

Latest base-merge validation on both `net10.0` and `net48`, Release:

- Import helpers and argument-binding regressions: 51 passed per
framework.
- Batch/fluent import, generated-state overlays, runtime/startup
lifecycle, cross-source references, routing, reference snapshots, and
node-authoring selection: 344 passed per framework.
- Merge-specific cases cover available Method parents with
forward/inverse and URI/index references while retaining an
application-owned `NodeId` handle.
- The unchanged upstream `NodeSetImportIntegrationTests.cs:210` CA1861
warning remains; no diagnostic suppression or unrelated cleanup was
added.

Release CI-fix validation on both `net10.0` and `net48`:

- `CustomCertificateGroupIntegrationTest`: 4 passed, including the
unchanged CI failure.
- `UANodeSetHelpersTests`: 8 passed, including imported security
metadata.
- `StartupRuntimeNodeSetLifecycleTests`: 5 passed.

Earlier Debug merge and review validation on both frameworks:

- Lifecycle, RuntimeNodeSet, fluent, and OperationContext selection
after merging `978e6d1af`: 638 passed per framework.
- Startup/transport-binding and RuntimeNodeSet integration selection
after merging `017856f4a`: 28 passed per framework.
- Public exception contract and duplicate-registration selection in
`fbf08428b`: 6 passed per framework.

Targeted test invocations rebuilt the affected projects successfully.
The full `UA.slnx` suite was not rerun locally.

## 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.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bb8a834-7695-4ae6-84cb-2471c76d1b12
 2026-09-08 11:11:53 +00:00
Marc Schier a7c776e8d4 Align the WoT implementation with spec: type binding, removed vocabulary, Alarms and Conditions (#4225) 
# Description

Aligns the WoT implementation with the normative changes in the OPC UA
WoT
specification drafts that the stack had not picked up, and fixes the
defects
that alignment surfaced.

The driving artifact is

[OPCF-Members/spec-drafts#19](https://github.com/OPCF-Members/spec-drafts/pull/19),
*"bind a projected node to an already-loaded type, by name or by
NodeId"*, which
landed on the spec `main` after this repository last tracked it.

## 1. Bind a projected node to an existing type (Binding §5.2.1)

§5.2.1 lets a document state that the node it projects is an instance of
a type
that **already exists** — one a companion model defines and the Server
has
loaded, or one a sibling document of the same conversion defines — so a
converter binds to that type instead of defining a second type of the
same
shape.

The converter hardcoded `HasTypeDefinition` to `BaseObjectType`, so a
document
that named its type projected an **untyped** node: a client browsing for
the
companion type would not find it, and nothing said why.

Both forms are now implemented, resolved against the §5.1.5 **local
context** —
the other documents being converted together first, a loaded
AddressSpace as the
fallback. `IWotNodeResolver` supplies that context (is a namespace held;
what
does a qualified BrowseName match; what does an ExpandedNodeId
identify),
`WotCompositeNodeResolver` composes implementations in the specified
order, and
`NullWotNodeResolver` is the default that holds nothing. The converter
needed no
new async surface: the binding is resolved in a pre-resolve pass,
exactly as
Thing references already are, and the synchronous core consumes the
result.

The §5.2.1 table is honoured: a unique name binds; the link settles an
ambiguous
name; a name that resolves to nothing while the link resolves is
**invalid**
(that is a mistake in the name, not a shorthand for the identifier); two
forms
resolving to different Nodes are invalid; a resolved type of the wrong
NodeClass
is invalid. A binding naming a type the local context does not hold
**fails**
rather than falling back to `BaseObjectType` — a silently mistyped node
is worse
than a reported failure.

A compact name is told from ordinary `@type` annotation **by
namespace**, not by
whether the lookup succeeds, so `saref:TemperatureSensor` on a Server
that has
never heard of SAREF stays the annotation it always was.

The definitive form — a `ua:HasTypeDefinition` link whose
`href` is the ExpandedNodeId of the type. An ExpandedNodeId matches
exactly one
Node or none, so it needs no lookup.

A Node has exactly one `HasTypeDefinition`, so:

- two such links → `AmbiguousTypeBinding` (6019), and the root keeps the
default
  rather than being bound to a guess;
- a link with no usable identifier → `InvalidTypeBinding` (6020);
- no binding at all → the `BaseObjectType` default is unchanged.

## 2. Drop the six vocabulary terms `#19` removed

`#19` deleted `uav:capability`, `uav:componentModel`, `uav:reference`,
`uav:congruentType`, `uav:congruentTypeName` and `uav:nameNamespace`
from the
context, the JSON Schema and the ontology. A link now names its
ReferenceType
directly in `rel`, so the three link relations become `ua:HasInterface`,
`ua:HasComponent` and `ua:NonHierarchicalReferences`, and the
congruent-type
pair is superseded by the §5.2.1 type binding.

Their validation and mapping are removed; they are now ordinary residue
like any
other unrecognised term.

### A real gap this uncovered

The converter knew **eight** ReferenceTypes by name and had neither
`NonHierarchicalReferences` (`i=32`) nor `HasInterface` (`i=17603`) — so
the two
relations that *replace* `uav:reference` and `uav:capability` could not
resolve
at all. That includes example 02, which this repository vendors as a
golden
asset and which `#19` rewrote to use them. Both are now in the map.

`uav:congruentType` was also the only term that redirected one reference
to
another while resolving a link target, so `ResolveTargetNodeIdAsync` no
longer
needs a loop. It resolves once and reports an unresolved target; the
resolution
context is still entered and the bytes still counted, so the
per-conversion
document, depth and byte bounds are unchanged.

## 3. Reject an out-of-range event severity instead of clamping it

`uav:severity` had grown in this implementation without being a
specification
term. Rather than drop it, it is proposed upstream in

[OPCF-Members/spec-drafts#21](https://github.com/OPCF-Members/spec-drafts/pull/21),
which defines it with an explicit rule: the value **shall** be in the
OPC 10000-5
range 1..1000, and a consumer **shall not** silently clamp it.

The implementation clamped, so a document asking for severity 5000 was
accepted
and published as 1000 — the mistake was hidden while what the author
asked for
was changed. An affordance carrying an out-of-range severity is now
skipped,
with the reason logged, and its half-built event type and
`GeneratesEvent`
reference are removed.

### Defect found while making that change

The occurrence-time path read `ValidateSeverity(severity) ??
tag.Severity`.
Because `ValidateSeverity(null)` returns the 500 default, it **never**
fell back
to the affordance's authored severity: an occurrence reported without a
severity
published 500 rather than the authored 900. Build-time validation and
the
occurrence-time choice are now separate methods.

## 4. Vendored examples and honest coverage claims

Examples 21 and 22 were missing from the vendored golden assets.
Converting
example 22 is now the end-to-end check for §5.2.1 — the specification's
own
example rather than a fixture written to match the implementation.

`docs/WotBindings.md` claimed ten of the eleven conformance units and
silently
omitted **WoT-ConditionMapping**, and said "twenty worked examples". The
count
is corrected, and WoT-ConditionMapping is now **covered** — see §5
below.

## 5. Alarms and Conditions (Binding §13)

The last unclaimed conformance unit. A projected Condition event now
derives
from the ConditionType it names rather than from `BaseEventType`;
falling back
would lose the Condition state model entirely, leaving a Client unable
to tell
an alarm from an ordinary event.

The two forms follow the hint-plus-pin pattern of §5.3:
`uav:conditionTypeId`
is definitive and wins, `uav:conditionType` is a readable hint resolved
for the
four ConditionTypes §13.1 scopes. An unpinned name outside that set is
reported
rather than guessed.

Four conformance rules are enforced, each because breaking it yields a
document
a consumer can read but cannot act on:

| Rule | Section | Diagnostic |
| --- | --- | --- |
| A Condition event declares `EventId` in its `data` | 13.3 |
`ConditionEventIdMissing` |
| `uav:conditionAction` is inside its closed set | 13.2 |
`InvalidConditionAction` |
| `uav:actsOn` names a Condition event in the same document | 13.4 |
`InvalidConditionTarget` |
| `Acknowledge`/`Confirm`/`AddComment` declare an `EventId` input | 13.4
| `ConditionActionInputMissing` |

`Enable` and `Disable` act on the Condition instance rather than one
occurrence
and are deliberately exempt from the last rule, pinned by a negative
test.
Shelving, suppression, dialog conditions and `ConditionRefresh` stay out
of
scope, as §13.1 scopes the mapping.

## 6. The sibling half of the §5.1.5 local context

`SnapshotWotNodeResolver` indexes the registry snapshot a conversion
runs over
and is wired into `WotNodeSetDocumentConverter`, so a document naming a
type
another document in the same registry projects resolves with no
AddressSpace at
all. Only Thing Models are indexed: a Thing Model projects a
`UAObjectType` and
is a valid target, a Thing Description projects an instance and never
is.

Identity comes from the new public
`WotNodeSetConverter.TryDescribeProjectedType`,
which applies exactly the rules the conversion uses, so an index entry
and the
projected node cannot drift apart. Ambiguity is preserved rather than
resolved:
two siblings sharing a qualified name yield both matches so the caller
can
report it.

## Deliberately not in this PR

- **The AddressSpace-backed `IWotNodeResolver`.** The abstraction, the
composite, the null default and the sibling-document implementation ship
here; the AddressSpace-backed half needs a cached type-hierarchy walk
behind
a server `OperationContext` and follows separately. Until it exists a
host
gets the sibling context plus the null fallback, which reports a binding
as
  unresolved rather than mistyping it.
- **Two §5.2.1 obligations that need the address space**: not
duplicating a
  mandatory instance declaration of the resolved type, and rejecting
  disagreement with an instantiated Thing Model.
- **Connectivity §7.3 parent resolution.**

Each is tracked; none is faked here.

## Related Issues

There is no tracking issue: this came out of a
spec-versus-implementation audit
rather than a reported defect, and the driving artifacts are the two
upstream
spec PRs linked above. Happy to open one if maintainers prefer — the
§5.2.1
follow-up in particular is worth an ADR, since the local-context
resolver is a
new injectable provider spanning two assemblies.

## 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 detail

| | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Types.Tests` | 8573 / 8573 | 8555 / 8555 |
| `Opc.Ua.WotCon.Tests` | 1075 / 1075 | 1075 / 1075 |

Full `UA.slnx` build: **0 errors, 0 warnings** across all target
frameworks,
re-verified after `master` was merged into the branch.

Every new test was mutation-verified: the type-binding tests against
three
mutations (revert to the hardcoded default, drop the ambiguity check,
accept an
empty `href`), the severity tests against reinstating the clamp, and the
ReferenceType additions against removing them from the map. Each
mutation failed
the suite before the change was kept.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
 2026-08-14 07:55:33 -07: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 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00