lmxopcua/archreview/01-core-composition.md

Architecture Review — Core Composition Pipeline and Core Libraries

Date: 2026-07-12 Commit: f6eaa267 (master, clean tree) Note: This updates the 2026-07-08 review at 9cad9ed0. Since that baseline the arch-review remediation branches merged to master; the pieces landing in this domain are: the IDriverCapabilityInvoker/IDriverCapabilityInvokerFactory seam in Core.Abstractions + DriverCapabilityInvokerFactory in Core (bacea1a4, task #10), per-instance ResilienceConfig read-path plumbing through the deploy artifact (75403caa, task #13), the reflection-exhaustive deferred-sink forwarding guard in Commons.Tests (a65c2ced, task #6), the typed SBR option in Cluster (a81dea10 + premise correction eaf78aad), the OTOPCUA0001 analyzer wiring (f0082af5), IScriptCacheOwner in Commons (7fd44f0f), and a fixdocs XML-doc sweep (3b5ef439).

Reviewer scope:

• src/Core/ZB.MOM.WW.OtOpcUa.Core (driver hosting, resilience, stability, authorization trie)
• src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions (driver capability interfaces, PollGroupEngine, historian seams, the new invoker seam)
• src/Core/ZB.MOM.WW.OtOpcUa.Commons (cluster message contracts, deferred sinks, NodeId scheme, telemetry)
• src/Core/ZB.MOM.WW.OtOpcUa.Configuration (EF config persistence, generation-sealed local cache, draft validation)
• src/Core/ZB.MOM.WW.OtOpcUa.Cluster (Akka bootstrap, role info, ServiceLevel, SBR options)
• Plus the address-space composition pipeline itself — AddressSpaceComposer / AddressSpacePlanner / AddressSpaceApplier

Scope note (location drift). The review brief places the composition pipeline in ZB.MOM.WW.OtOpcUa.Core, but since the Phase7→AddressSpace rename it lives in src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/ (AddressSpaceComposer.cs, AddressSpacePlan.cs, AddressSpaceApplier.cs), with its tests under tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/. The Core project's own OpcUa/ folder still holds only the retired EquipmentNodeWalker and an unused GenericDriverNodeManager (see U-1). The pipeline was reviewed in full regardless. The composition pipeline itself received zero code changes in the 9cad9ed0→f6eaa267 window; the domain's delta is concentrated in Core/Resilience, Core.Abstractions, Cluster, and the Runtime dispatch layer that consumes the new seam.

---

Prior-finding status (9cad9ed0 → f6eaa267)

Every prior finding was re-verified against the current code, not the STATUS.md claims.

ID	One-line description	Verified status
S-1	Applier swallows every sink failure; deploy reported applied even when broken	STILL OPEN — Safe* helpers unchanged (AddressSpaceApplier.cs:373–383, 612–622); AddressSpaceApplyOutcome (:691) still has no failure fields; rebuilt = true still set after SafeRebuild() (:148–153). STATUS.md itself lists 01/S-1 under "Suggested next".
S-2	PollGroupEngine.Unsubscribe blocks up to 5 s per subscription	STILL OPEN — task.Wait(TimeSpan.FromSeconds(5)) at PollGroupEngine.cs:109.
S-3	No disposed guard: Subscribe/RegisterAsync race DisposeAsync	STILL OPEN — no _disposed flag in PollGroupEngine.cs or Core/Hosting/DriverHost.cs.
S-4	ScheduledRecycleScheduler catch-up recycle storm after a stall	STILL OPEN — _nextRecycleUtc += _recycleInterval (ScheduledRecycleScheduler.cs:82), no fast-forward past utcNow.
S-5	GenericDriverNodeManager re-walk teardown unsynchronized	STILL OPEN (moot) — code unchanged; the #10 triage added an OTOPCUA0001 pragma (GenericDriverNodeManager.cs:69) explicitly memorializing "zero production references", strengthening the U-1 delete case.
P-1	Compose parses each tag's TagConfig JSON four times	STILL OPEN — four JsonDocument.Parse sites unchanged (AddressSpaceComposer.cs:541, 606, 638, 670 from :418–433).
P-2	Per-operation allocations on the authorization hot path	STILL OPEN — ldapGroups.ToHashSet(...) per call at PermissionTrie.cs:42.
P-3	PollGroupEngine: task-per-subscription, no coalescing	STILL OPEN — engine unchanged.
P-4	Per-write allocation in the non-idempotent write path	STILL OPEN — options record + Dictionary per call at CapabilityInvoker.cs:117–131 (and now production-unreachable — see S-8).
C-1	Byte-parity TagConfig parsing replicated in four projects	STILL OPEN — all four copies + "MUST parse identically" comments intact; #13 added a fifth small parity-coupled read (DeploymentArtifact.TryReadSpec ResilienceConfig) though that one is a trivial string read.
C-2	Core depends on Configuration; EF entities are the domain model	STILL OPEN (accepted-risk) — Core.csproj:16 still references Configuration.
C-3	DraftValidator re-derives NodeId scheme + hard-codes "GalaxyMxGateway"	STILL OPEN — DraftValidator.cs:48, 65, 75–97 unchanged.
C-4	Composer violates purity contract with Trace.TraceWarning	STILL OPEN — AddressSpaceComposer.cs:493.
C-5	Inconsistent duplicate-key defensiveness in Compose	STILL OPEN — last-wins deviceHostById (:357–363) vs throw-on-dupe ToDictionary (:401–402, 453).
C-6	Vestigial Akka package reference in Commons	STILL OPEN — Commons.csproj:9.
C-7	Stale scaffold-era XML docs on load-bearing classes	PARTIALLY FIXED — fixdocs 3b5ef439 converted CapabilityInvoker's duplicated docs to <inheritdoc/>, but both flagged narratives survive verbatim: the applier's F14-scaffold doc ("For now we record the work", "the SDK adapter that lands in F10b will decide…" — AddressSpaceApplier.cs:7–26) and DriverHost's "Phase 2 via named-pipe RPC" promise (Core/Hosting/DriverHost.cs:5–10).
U-1	Dead/dormant code retained: GenericDriverNodeManager, EquipmentNodeWalker, TryParseRelayBody	STILL OPEN — all three present (Core/OpcUa/ lists both classes + IdentificationFolderBuilder; EquipmentScriptPaths.cs:164). Evidence grew: the analyzer triage now documents GDNM as test-only scaffolding in a pragma.
U-2	Tier C recycle machinery has no IDriverSupervisor implementation	STILL OPEN — repo-wide search still finds zero concrete implementations; the new DriverResilienceOptionsParser even validates RecycleIntervalSeconds as "Tier C only" for a tier that cannot execute (DriverResilienceOptionsParser.cs:101–110).
U-3	Continuous-historization initial interest set empty at spawn	PARTIALLY FIXED / RE-ANALYSED — the recorder is still spawned with Array.Empty<string>() (Runtime/ServiceCollectionExtensions.cs:277), but re-analysis shows restart convergence is mechanically present: DriverHostActor.RestoreApplied (DriverHostActor.cs:1293–1320) fires RebuildAddressSpace on bootstrap, OpcUaPublishActor.HandleRebuild diffs against an empty _lastApplied baseline → a full-add plan → Apply → FeedHistorizedRefs posts the complete set. The prior claim "historizes nothing until the next deploy" was too strong. What remains is exactly CLAUDE.md Known Limitation 2 (reworded by the remediation docs branch): no restart-convergence test and no live end-to-end verification. Kept in the body, downgraded.
U-4	Additive-only ACL model (no Deny)	STILL OPEN (documented v2.0 gap) — PermissionTrie.cs:13–17 unchanged.
U-5	Test coverage strong but unevenly distributed	PARTIALLY IMPROVED — new suites landed: SplitBrainResolverActivationTests (Cluster.Tests, now 4 files), DriverCapabilityInvokerFactoryTests (148 lines), pipeline-builder logging tests, and the Commons DeferredSinkForwardingReflectionTests guard. But ClusterRoleInfo — the only concurrency-bearing Cluster class — still has zero tests, and the S-1/S-3 surfaces remain untestable because they don't exist. Kept in the body, narrowed.

Net: 0 of the 5 stability findings, 0 of the 4 performance findings, and 6 of the 7 convention findings are unchanged; the remediation wave targeted other domains' Criticals and landed new code in this domain rather than fixing this domain's backlog. That new code is reviewed below (S-6/S-7/S-8, U-6, C-8).

---

Architecture Overview

Composition pipeline (deploy → address space)

The pipeline is a clean three-stage compose/diff/apply design (unchanged this window):

1. AddressSpaceComposer.Compose (OpcUaServer/AddressSpaceComposer.cs:290) — a pure static projection from EF config entities (UnsArea, UnsLine, Equipment, DriverInstance, Tag, VirtualTag, Script, ScriptedAlarm, Device, Namespace) into an AddressSpaceComposition: sorted lists of UnsAreaProjection/UnsLineProjection/ EquipmentNode/DriverInstancePlan/EquipmentTagPlan/EquipmentVirtualTagPlan/ EquipmentScriptedAlarmPlan. Tag intent (driver FullName, native-alarm object, historize flags, array shape) is parsed out of the schemaless Tag.TagConfig JSON here. VirtualTag scripts get {{equip}} token substitution and dependency-ref extraction via the shared Commons.Types.EquipmentScriptPaths helper. Everything is ordinally sorted so the composition is deterministic — the foundation of the "byte-parity" contract with the artifact-decode mirror in Runtime/Drivers/DeploymentArtifact.cs.

2. AddressSpacePlanner.Compute (OpcUaServer/AddressSpacePlan.cs:96) — a pure diff of two compositions keyed on stable logical ids, using record value-equality as the changed check (with hand-written Equals/GetHashCode on the plans carrying IReadOnlyList members so a no-op redeploy diffs empty). Emits Added/Removed/Changed sets per entity class plus UNS folder renames. AddressSpacePlan.IsEmpty is the short-circuit gate OpcUaPublishActor uses.

3. AddressSpaceApplier.Apply (OpcUaServer/AddressSpaceApplier.cs:73) — the only side-effecting stage. Decides between a full structural rebuild (any topology change) and a surgical in-place update (ISurgicalAddressSpaceSink.UpdateTagAttributes / UpdateFolderDisplayName for whitelisted node-irrelevant deltas — the F10b optimization), falling back to rebuild when the sink lacks the capability or any surgical call fails. After the address-space work it dispatches two non-blocking hooks: fire-and-forget historian tag provisioning (IHistorianProvisioning.EnsureTagsAsync) and the historized-ref delta feed to the continuous-historization recorder (IHistorizedTagSubscriptionSink.UpdateHistorizedRefs). Separate Materialise* passes (Hierarchy / EquipmentTags / VirtualTags / ScriptedAlarms / DiscoveredNodes) re-derive nodes from the composition after a rebuild; NodeIds are folder-scoped (Commons.OpcUa.EquipmentNodeIds: {equipment}/{folder}/{name}), never the driver FullName, to avoid collisions across identical machines.

The sink boundary (Commons.OpcUa.IOpcUaAddressSpaceSink) keeps the whole pipeline SDK-free: production binds SdkAddressSpaceSink via the late-swap DeferredAddressSpaceSink (actors resolve the wrapper at DI time, the OPC UA hosted service swaps the real sink in after StandardServer starts); dev/tests bind the NullOpcUaAddressSpaceSink no-op. The forwarding contract of that wrapper is now guarded by a reflection-exhaustive test (Commons.Tests/OpcUa/ DeferredSinkForwardingReflectionTests.cs) — see C-8 for its one blind spot.

The new resilience dispatch seam (landed this window)

The Phase 6.1 CapabilityInvoker, which the 2026-07-08 review cycle found built-but-never-wired (RESILIENCE-DISPATCH-GAP), is now live in production dispatch via a Polly-free seam that mirrors IDriverFactory:

• Core.Abstractions/IDriverCapabilityInvoker.cs — the invoker abstraction (3 methods matching CapabilityInvoker's shapes exactly) plus NullDriverCapabilityInvoker, a genuine pass-through.
• Core.Abstractions/IDriverCapabilityInvokerFactory.cs — per-instance factory abstraction plus the Null factory.
• Core/Resilience/DriverCapabilityInvokerFactory.cs — the concrete factory: resolves the driver-type tier, layers per-instance ResilienceConfig JSON on the tier defaults via DriverResilienceOptionsParser.ParseOrDefaults (logging the parse diagnostic), calls builder.Invalidate(instanceId) so a respawn with changed options rebuilds cached pipelines, and snapshots the options once per Create so the per-call accessor is allocation-free.
• The Host's DriverFactoryBootstrap registers the factory (Host/Drivers/DriverFactoryBootstrap.cs:63–70), Runtime resolves it like IDriverFactory (Runtime/ServiceCollectionExtensions.cs:365–366), DriverHostActor.SpawnChild creates one invoker per child (DriverHostActor.cs:1630), and DriverInstanceActor wraps its six dispatch sites. DriverSpawnPlanner forces a respawn on any ResilienceConfig change (DriverSpawnPlan.cs:40–46); DeploymentArtifact.TryReadSpec reads the column null-safely from the artifact (DeploymentArtifact.cs:168–171). The OTOPCUA0001 analyzer is wired tree-wide and knows the interface as a wrapper home, so an unwrapped capability call is a build error.

The layering is genuinely clean — Runtime stays Polly-free, null objects keep every test harness byte-identical, and the invalidate/respawn pairing closes the options-blind-cache trap. The new findings below (S-6, S-7, S-8, U-6) are all second-order issues inside this new machinery, not problems with its architecture.

Supporting Core libraries

• Core/Hosting — DriverFactoryRegistry (driver-type → factory + GetTier), DriverFactoryRegistryAdapter, DriverHost (id → IDriver lifecycle registry).
• Core/Resilience — Polly pipelines cached per (DriverInstanceId, HostName, Capability) (DriverResiliencePipelineBuilder, now with retry/breaker event logging as the interim observability surface), executed via CapabilityInvoker (with the non-idempotent-write no-retry override), built per instance by DriverCapabilityInvokerFactory, fanned out per host by AlarmSurfaceInvoker, observed by DriverResilienceStatusTracker for Admin /hosts.
• Core/Stability — MemoryTracking, MemoryRecycle and ScheduledRecycleScheduler (Tier C recycle via IDriverSupervisor), WedgeDetector.
• Core/Authorization — generation-sealed PermissionTrie per (ClusterId, GenerationId) cached in PermissionTrieCache (CAS-pruned), walked by TriePermissionEvaluator against per-session UserAuthorizationState.
• Commons — immutable record message contracts for the Akka DPS topics, strongly-typed ids, the deferred sink/publisher pair, EquipmentScriptPaths, OtOpcUaTelemetry, and now Engines/IScriptCacheOwner (the apply-boundary compile-cache drop capability for IVirtualTagEvaluator implementations).
• Configuration — OtOpcUaConfigDbContext (26 entities), the generation-sealed LiteDB fallback cache, ResilientConfigReader, DraftValidator.
• Cluster — AddOtOpcUaCluster / WithOtOpcUaClusterBootstrap (now with the extracted, testable BuildClusterOptions setting KeepOldestOption { DownIfAlone = true } explicitly — the #11 premise correction is accurately reflected in the akka.conf comment, the XML doc, and SplitBrainResolverActivationTests), ClusterRoleInfo, ServiceLevelCalculator, RoleParser.

Dependency direction: Core.Abstractions (leaf) ← Configuration ← Core; Commons (leaf, Akka+Audit packages) ← Cluster. The composer/applier in OpcUaServer reference both Configuration (entities) and Commons (sink contracts).

---

Findings

1. Stability

S-1 — Applier swallows every sink failure; a deploy is reported applied even when the address space is broken — High (unchanged since 2026-07-08)

AddressSpaceApplier wraps every sink call in catch-all "Safe" helpers that log and continue: SafeEnsureFolder/SafeEnsureVariable (AddressSpaceApplier.cs:612–622), SafeRebuild (AddressSpaceApplier.cs:373–383), SafeWriteAlarmCondition/SafeMaterialiseAlarmCondition (AddressSpaceApplier.cs:677–687). AddressSpaceApplyOutcome (AddressSpaceApplier.cs:691) carries only Added/Removed/Changed counts and RebuildCalled — no failure count. If RebuildAddressSpace() throws, rebuilt is still reported true (AddressSpaceApplier.cs:148–153) and the outcome flows back to the deploy coordinator as a success; the deployment seals while the running server holds a partially-materialised (or entirely stale) address space. Individual EnsureVariable failures during a materialise pass likewise vanish into per-node Warnings.

The never-fail-a-deploy posture is defensible for the detached hooks (provisioning, historized-ref feed — both correctly isolated), but structural materialisation failures are the deploy's core contract. This is the domain's highest-priority open item; STATUS.md already queues it as a "failure-visibility High".

Recommendation: add FailedNodes/RebuildFailed to AddressSpaceApplyOutcome, propagate into the ApplyAck/DeploymentFailed decision and the audit log, and escalate SafeRebuild failure to at least a degraded ack. Tests should assert the failure surface (the fixture sink can throw).

S-2 — PollGroupEngine.Unsubscribe blocks the calling thread up to 5 s per subscription — Medium (unchanged)

StopState does a synchronous task.Wait(TimeSpan.FromSeconds(5)) (Core.Abstractions/PollGroupEngine.cs:99–112). Unsubscribe is the teardown path drivers call from OPC UA subscription-management callbacks and actor message handlers; a reader stuck in a slow network call turns every unsubscribe into a 5-second stall of the calling thread (and N stalls when tearing down N subscriptions serially). DisposeAsync gets this right (parallel cancel, awaited WhenAll with one shared timeout, PollGroupEngine.cs:213–236); the single-subscription path does not.

Recommendation: add an UnsubscribeAsync (or make Unsubscribe cancel + hand the await to a background drain), keeping the "no callback after teardown" guarantee via the existing LoopTask/CTS-dispose ordering.

S-3 — PollGroupEngine.Subscribe has no disposed guard — Low (unchanged)

Subscribe racing DisposeAsync can insert a fresh SubscriptionState after the dispose loop has snapshotted _subscriptions.Values (PollGroupEngine.cs:73–84 vs 213–236), leaking a live poll loop with no owner. Same pattern in DriverHost.RegisterAsync vs DisposeAsync (Core/Hosting/DriverHost.cs:53–71 vs 92–106). Both are shutdown-window races, unlikely in practice but cheap to close with a _disposed flag checked inside the lock / before insert.

S-4 — ScheduledRecycleScheduler catch-up recycle storm after a stall — Low (unchanged)

TickAsync advances _nextRecycleUtc by exactly one interval per fire (Core/Stability/ScheduledRecycleScheduler.cs:74–82). If the host was suspended (VM pause, long outage) across K intervals, the next K ticks each trigger a full Tier C process recycle back-to-back. Recommendation: fast-forward _nextRecycleUtc past utcNow after a fire.

S-5 — GenericDriverNodeManager re-walk teardown is not synchronized — Low (unchanged; dormancy now formally documented)

BuildAddressSpaceAsync tears down the previous alarm forwarder and clears _alarmSinks non-atomically (Core/OpcUa/GenericDriverNodeManager.cs:58–73). Still moot: the #10 analyzer triage added a pragma at GenericDriverNodeManager.cs:69 stating outright that this class has "zero production references" and is exercised only by its own tests — fold this into the U-1 decision.

S-6 — NEW: a valid-JSON ResilienceConfig with an out-of-range timeout permanently fails every guarded capability call — violates the parser's own never-brick contract — High

DriverResilienceOptionsParser.ParseOrDefaults defends against malformed JSON (catch (JsonException) at DriverResilienceOptionsParser.cs:71–76), unknown capability names, and a misapplied Tier-C recycle interval — but performs no range validation on the numeric override values it merges (DriverResilienceOptionsParser.cs:91–95): TimeoutSeconds, RetryCount, and BreakerFailureThreshold pass straight through. The pipeline builder then does builder.AddTimeout(new TimeoutStrategyOptions { Timeout = TimeSpan.FromSeconds(policy.TimeoutSeconds) }) (DriverResiliencePipelineBuilder.cs:111–114). Polly v8 range-validates TimeoutStrategyOptions.Timeout (and RetryStrategyOptions.MaxRetryAttempts) at pipeline build time — so "timeoutSeconds": 0, a negative value, or an absurdly large one throws a ValidationException inside the _pipelines.GetOrAdd factory on the driver's first capability call, and on every subsequent call (a throwing factory caches nothing). Result: subscribe, discover, write, and alarm-ack all fail persistently for that driver while the driver itself is healthy — precisely the "misconfigured ResilienceConfig should never brick a working driver" outcome the parser's remarks (DriverResilienceOptionsParser.cs:30–33) promise to prevent.

This is operator-reachable: the AdminUI's ResilienceFormModel binds TimeoutSeconds as a bare int? with no range validation anywhere in the form model (AdminUI/Components/Shared/Drivers/ResilienceFormModel.cs:32–37), so typing 0 into the timeout field, publishing, and deploying produces the failure. And because #13 correctly forces a respawn on any ResilienceConfig change, the bad config takes effect immediately.

Recommendation: clamp/reject out-of-range values in ParseOrDefaults (the diagnostic out-parameter already exists — TimeoutSeconds < 1 → tier default + diagnostic, cap at Polly's max; RetryCount < 0 or > 100 → clamp), add a parser test per field, and mirror the same bounds as form-side validation in ResilienceFormModel. Defense in depth belongs in the parser: the JSON column is writable by more paths than the AdminUI form.

S-7 — NEW: respawn Invalidate race can permanently re-cache a stale-options pipeline — Low

DriverCapabilityInvokerFactory.Create calls _builder.Invalidate(driverInstanceId) (DriverCapabilityInvokerFactory.cs:71) before returning the new invoker, and DriverHostActor processes plan.ToStop before plan.ToSpawn in the same handler (DriverHostActor.cs:1278–1280) — but StopChild is an asynchronous actor stop. An in-flight capability call still executing inside the old child's handler can reach GetOrCreate after the new spawn's Invalidate has run, re-populating the cache with a pipeline built from the old options snapshot. Because the builder deliberately ignores options on a cache hit (DriverResiliencePipelineBuilder.cs:66–79), the new invoker then serves that stale pipeline indefinitely — the transient race has a permanent consequence, silently undoing the operator's ResilienceConfig change for the affected (host, capability) keys. The window is small (one in-flight handler) but the failure is invisible.

Recommendation: make staleness impossible rather than racing it — include an options generation (or the options reference) in PipelineKey, or have Create capture a per-invoker generation the old invoker can no longer write into. Alternatively, defer the new spawn until the old child's Terminated is observed (the actor-name generation counter shows the respawn already tolerates the old child lingering).

S-8 — NEW: production dispatch hardcodes isIdempotent: true; the non-idempotent write safeguard is unreachable — Medium

The only production call to ExecuteWriteAsync is DriverInstanceActor.HandleWriteAsync passing isIdempotent: true unconditionally (Runtime/Drivers/DriverInstanceActor.cs:597–601); a repo-wide search finds no caller passing false. The !isIdempotent no-retry branch in CapabilityInvoker.ExecuteWriteAsync (CapabilityInvoker.cs:117–131) — the mechanism Phase 6.1 built specifically so non-idempotent device writes are never retried — is production-dead code. The inline comment's reasoning ("an OPC UA attribute set is idempotent") holds for plain value writes, but drivers surface write-through targets where a duplicated write is not harmless (command/pulse coils, increment registers, Galaxy supervisory writes), and the WriteIdempotentAttribute the invoker was designed around is never consulted at the seam. Today this is behavior-neutral (every tier's Write default is RetryCount: 0 — DriverResilienceOptions.cs:83, 94, 105), but the entire point of #13 is that an operator can now switch Write retries on per instance — and the moment one does, all of that driver's writes retry, with the no-retry safety valve unreachable.

Recommendation: either thread the tag-level idempotence signal (from WriteIdempotentAttribute / driver metadata) through WriteAttribute to the dispatch site, or — if all-writes-idempotent is the deliberate v1 posture — delete the isIdempotent parameter from the seam and document the invariant, so the dead branch (and its per-write allocation, P-4) stops implying a guarantee that isn't wired.

Positive observations. The failure-mode engineering in Configuration/LocalCache remains exemplary (fail-closed GenerationSealedCache, atomic pointer swaps, the documented LiteDB traps); ResilientConfigReader and PermissionTrieCache.Prune are unchanged and correct. The applier's two historian hooks are properly detached (AddressSpaceApplier.cs:255–300). The new remediation code is itself defensively built: TryReadSpec reads ResilienceConfig null-safely and the spec survives absent/null values (DeploymentArtifact.cs:168–181); the factory logs parse diagnostics instead of throwing; Invalidate is a correct enumerate-and-TryRemove pass over the concurrent dictionary; DriverSpawnPlanner's respawn-on-ResilienceConfig-change with in-place delta for pure DriverConfig changes (DriverSpawnPlan.cs:40–46) is exactly the right split; and the DriverInstanceActor wrapping preserved the hard-won no-ConfigureAwait(false)-on-the-actor-await rule with an explicit comment that the invoker's internal ConfigureAwait(false) does not leak to the caller (DriverInstanceActor.cs:290–300) — backed by a real-invoker actor-context test.

2. Performance

P-1 — Compose parses each tag's TagConfig JSON four times — Medium (unchanged)

ExtractTagFullName, ExtractTagAlarm, ExtractTagHistorize and ExtractTagArray each do their own JsonDocument.Parse of the same blob per tag (AddressSpaceComposer.cs:418–433 calling :536–686). A compose over a 10k-tag fleet performs 40k JSON parses (plus the same again on the artifact-decode side). Compose runs on every deploy and every diff baseline, not on the data hot path, so this is a scalability tax rather than a latency bug today.

Recommendation: parse once per tag into a small TagConfigIntent record (which also collapses the byte-parity duplication — see C-1).

P-2 — Per-operation allocations on the authorization hot path — Medium (unchanged)

PermissionTrie.CollectMatches allocates a fresh HashSet<string> from the session's LDAP groups plus a List<MatchedGrant> on every call (Core/Authorization/PermissionTrie.cs:42–45), and TriePermissionEvaluator.Authorize runs per node-operation. A recursive browse or a large CreateMonitoredItems batch turns this into per-node garbage. UserAuthorizationState.LdapGroups is immutable per membership refresh — the case-insensitive set can be computed once per session version and reused.

P-3 — PollGroupEngine: one background task + independent timer per subscription, no cross-subscription batching — Medium (unchanged)

Each Subscribe spawns a dedicated Task.Run loop (PollGroupEngine.cs:73–84). Drivers that map each OPC UA monitored-item group (or each poll group) to a subscription get linear task/timer growth, and two subscriptions polling overlapping tag sets at the same interval issue duplicate protocol reads — the engine has no interval-bucketed scheduler or read coalescing. Fine at dozens of subscriptions; a scaling wall at hundreds per driver instance.

P-4 — Per-write allocation in the non-idempotent write path — Low (unchanged; now also dead code — see S-8)

CapabilityInvoker.ExecuteWriteAsync builds a new options record + Dictionary per non-idempotent write (Core/Resilience/CapabilityInvoker.cs:117–131) even though GetOrCreate keys only on (id, host, capability) and ignores the options after the first build. Since the only production caller now passes isIdempotent: true (S-8), this path is unreachable — resolve it together with S-8 (either cache the no-retry snapshot or delete the branch).

Positive observations. Pipeline resolution remains lock-free ConcurrentDictionary reads; the new factory snapshots options once per Create so the per-call accessor is allocation-free — an explicit hot-path improvement over re-parsing per call. The planner's diff is O(N) dictionary passes with deterministic sorted outputs; the surgical-apply path and its whitelist-via-with-expression technique (AddressSpaceApplier.cs:630–663) are unchanged and future-field-safe. The new resilience logging only attaches callbacks when a tracker or logger is present, so unit-test pipelines stay callback-free.

3. Conventions

C-1 — "Byte-parity by convention": TagConfig parsing replicated in four projects with comment-enforced sync — High (unchanged)

ExtractTagFullName (and friends) exist as deliberate copies in:

• OpcUaServer/AddressSpaceComposer.cs:536–551 (+ ExtractTagAlarm/ExtractTagHistorize/ExtractTagArray/TryExtractDeviceHost)
• Runtime/Drivers/DeploymentArtifact.cs (the artifact-decode mirror, per its own comments)
• Core/OpcUa/EquipmentNodeWalker.cs:193–208
• Configuration/Validation/DraftValidator.cs:60–71

Each copy carries a "MUST parse identically (byte-parity)" comment. The equality of the deployed address space across the live-compose and artifact-decode seams — and therefore the correctness of the diff/no-op-redeploy behavior — rests on humans keeping four JSON parsers in sync. The codebase already demonstrated the fix once: EquipmentScriptPaths (Commons/Types/EquipmentScriptPaths.cs) was created exactly to de-duplicate the ctx.GetTag extraction.

Recommendation: move the TagConfig/DeviceConfig intent parsing into Commons as a single TagConfigIntent.Parse(string) — also resolving P-1 — and keep one cross-seam parity test instead of N replicated implementations.

C-2 — Core depends on Configuration: EF entities and enums are the domain model — Medium (unchanged, accepted-risk)

ZB.MOM.WW.OtOpcUa.Core.csproj:16 references Configuration; PermissionTrie consumes Configuration.Enums, EquipmentNodeWalker consumes Configuration.Entities directly, and the composer's plan records are projections of EF entities. A deliberate, consistent choice — flagged as accepted-risk to document rather than refactor. Notably, the new remediation code got this boundary right: the invoker seam lives in Core.Abstractions precisely so Runtime avoids the Core→Polly closure — new code should keep preferring that pattern.

C-3 — DraftValidator re-derives the NodeId scheme and hard-codes a driver-type string — Medium (unchanged)

ValidateNoEquipmentSignalNameCollision re-implements the folder-scoped NodeId key by hand (Configuration/Validation/DraftValidator.cs:75–97) because Configuration cannot reference Commons.OpcUa.EquipmentNodeIds — a third copy of the {equipment}/{folder}/{name} scheme. ValidateGalaxyTagFullName hard-codes dtype != "GalaxyMxGateway" (DraftValidator.cs:48). Both are symptoms of the same missing shared-contract home as C-1.

C-4 — Composer violates its own purity contract with Trace.TraceWarning — Low (unchanged)

The class doc says "Same inputs → same outputs, no logging" (AddressSpaceComposer.cs:280–283), but the dangling-predicate-script skip emits Trace.TraceWarning (AddressSpaceComposer.cs:493–496) — the only System.Diagnostics.Trace usage in a Serilog/ILogger codebase, invisible in the production sinks. Either return skipped-alarm ids in the composition for the caller to log, or accept an injected logger and fix the doc.

C-5 — Inconsistent duplicate-key defensiveness inside Compose — Low (unchanged)

deviceHostById is built with a deliberate last-wins foreach and a comment explaining why ToDictionary (throw-on-dupe) would diverge from the decode side (AddressSpaceComposer.cs:357–363), yet driversById/namespacesById/scriptsById use plain ToDictionary (AddressSpaceComposer.cs:401–402, 453). Pick one posture; the defensive one is already argued for.

C-6 — Vestigial Akka package reference in Commons — Low (unchanged)

Commons.csproj:9 references the Akka package but no Commons source uses an Akka type (the new IScriptCacheOwner doesn't either — only doc comments mention actors). Remove it.

C-7 — Stale scaffold-era XML docs on load-bearing classes — Low (partially fixed)

The fixdocs sweep (3b5ef439) fixed CapabilityInvoker's doc duplication, but both narratives this finding flagged survive verbatim: AddressSpaceApplier's class doc still describes the F14 scaffold ("For now we record the work", "the SDK adapter that lands in F10b will decide…" — AddressSpaceApplier.cs:7–26) although both features shipped, and DriverHost's doc still promises "per-process isolation for Tier C … implemented in Phase 2 via named-pipe RPC" (Core/Hosting/DriverHost.cs:5–10), an architecture superseded by the out-of-repo gateways. In a codebase whose XML documentation is otherwise a genuine asset, stale top-of-class narratives are actively misleading.

C-8 — NEW: the deferred-sink forwarding guard's "exhaustive" interface detection is namespace-filtered — Low

DeferredSinkForwardingReflectionTests.DeferredAddressSpaceSink_implements_exactly_the_known_forwarding_interfaces — the guard-of-the-guard whose whole job is catching a new capability interface added to the wrapper — filters GetInterfaces() to i.Namespace == typeof(IOpcUaAddressSpaceSink).Namespace (Commons.Tests/OpcUa/DeferredSinkForwardingReflectionTests.cs:44), i.e. only ZB.MOM.WW.OtOpcUa.Commons.OpcUa. Commons already has sibling namespaces the wrapper could plausibly grow an interface from (Commons.Engines — home of the brand-new IScriptCacheOwner — and Commons.Types): a future forwarding interface declared outside Commons.OpcUa would be silently excluded from ForwardingInterfaces enforcement, re-opening exactly the F10b/PR#423 inert-capability trap class this guard exists to close. The per-method forwarding test and the DummyArg fabricator are otherwise well built (the DispatchProxy recorder auto-covers future members; bool-returning members return true so capability-sniffing short-circuits still record).

Recommendation: drop the namespace filter and instead exclude a small explicit allowlist of known non-forwarding interfaces (e.g. IDisposable), so any unknown interface trips the guard regardless of where it lives.

Positive observations. Naming and structure remain highly consistent, and the new code follows the house patterns exactly: the seam mirrors IDriverFactory, both null objects are private-ctor singletons, options records keep Validate()-style diagnostics, and every new public member is documented. Two convention-enforcement mechanisms landed that materially raise the floor: the OTOPCUA0001 analyzer is wired tree-wide (an unwrapped capability call is now a build error, with the one remaining pragma carrying a full rationale — GenericDriverNodeManager.cs:69), and the reflection forwarding guard turns the deferred-sink trap from a code-review convention into a failing test. The #11 premise correction is honest engineering — the SBR comments/docs now state that the typed option is reinforcing, not the activator, instead of quietly keeping the wrong war story.

4. Underdeveloped Areas

U-1 — Dead/dormant code retained in Core: GenericDriverNodeManager, EquipmentNodeWalker, TryParseRelayBody — Medium (unchanged; evidence strengthened)

• GenericDriverNodeManager (Core/OpcUa/GenericDriverNodeManager.cs) has no production references — now stated in the source itself by the #10 triage pragma ("zero production references", GenericDriverNodeManager.cs:69).
• EquipmentNodeWalker (Core/OpcUa/EquipmentNodeWalker.cs:160–165) self-declares "retained for unit-test support only" — ~280 lines plus IdentificationFolderBuilder and a full test suite for a path that cannot run in production. Its ExtractFullName copy is one of the four C-1 duplicates.
• EquipmentScriptPaths.TryParseRelayBody (Commons/Types/EquipmentScriptPaths.cs:164–172) serviced the deleted relay→alias converter; only tests call it.

Dormant code with green tests is worse than deleted code: it passes every gate while silently diverging from the live path — the review cycle's own RESILIENCE-DISPATCH-GAP episode proved the genre. Recommendation: delete all three (git preserves them), or move the walker into the test project if it must survive as a fixture.

U-2 — The entire Tier C recycle machinery has no IDriverSupervisor implementation — Medium (unchanged; now accreting further)

MemoryRecycle, ScheduledRecycleScheduler, and DriverResilienceStatusTracker.RecordRecycle all drive IDriverSupervisor.RecycleAsync, but a repo-wide search still finds zero concrete implementations. The gap is now growing supporting surface: DriverResilienceOptionsParser carefully gates RecycleIntervalSeconds to "Tier C only" (DriverResilienceOptionsParser.cs:101–110) and the AdminUI authors the field — configuration and validation for an execution path that does not exist. Either document it as speculative infrastructure for a future tier-migration workflow or prune it alongside U-1.

U-3 — Continuous-historization restart convergence: mechanically present, unverified — Low (downgraded from Medium; prior framing partially superseded)

Re-analysis this cycle shows the prior "after a restart the recorder historizes nothing until the next deploy" framing was too strong: DriverHostActor.RestoreApplied fires a RebuildAddressSpace on bootstrap (DriverHostActor.cs:1293–1320), and OpcUaPublishActor.HandleRebuild diffs the restored artifact against an empty _lastApplied baseline (OpcUaPublishActor.cs:290–340) — a full-add plan whose Apply feeds the complete historized set through FeedHistorizedRefs (AddressSpaceApplier.cs:310–353). The spawn-time empty set (Runtime/ServiceCollectionExtensions.cs:269–278) is therefore covered by the restore path, and the recorder is deliberately spawned before the applier wiring so the feed can reach it. What remains is exactly what CLAUDE.md Known Limitation 2 (as reworded by the remediation) says: no restart-convergence test asserting the recorder re-registers the deployed set after a process restart, and no live end-to-end verification. Given the subsystem's history of mechanically-present-but-inert paths, that test is cheap insurance and should be written.

U-4 — Additive-only ACL model (no Deny) is a documented v2.0 gap — Low (unchanged)

PermissionTrie is explicitly pure-union ("no explicit Deny in v2.0", Core/Authorization/PermissionTrie.cs:13–17): a broad cluster-root grant cannot be carved back at a sub-scope. Acceptable for the current flat-role deployments, but it bounds how fine-grained OT authorization can get — keep visible in the security docs.

U-5 — Test coverage: ClusterRoleInfo remains the riskiest untested seam — Low (narrowed from prior)

This cycle materially improved the domain's test posture: SplitBrainResolverActivationTests (Cluster.Tests), DriverCapabilityInvokerFactoryTests (invalidate-per-instance, malformed-config fallback, override-reaches-execution), pipeline-builder logging tests, and the Commons forwarding reflection guard all landed. Still missing: ClusterRoleInfo — the only concurrency-bearing class in the Cluster project (lock + subscriber actor + event fan-out) — has no test (Cluster.Tests still covers only pure classes plus the SBR option), and the S-1 applier failure surface and S-3 dispose races remain untested because the surfaces don't exist. An Akka TestKit harness for ClusterRoleInfo leader-change sequencing would close the riskiest untested seam.

U-6 — NEW: bulkhead is advertised, authored, parsed — and never applied — Medium

The resilience surface documents "retry / circuit-breaker / bulkhead / tracker telemetry" in the seam docs (IDriverCapabilityInvoker.cs:4–6), the analyzer's diagnostic message (UnwrappedCapabilityCallAnalyzer.cs:105), and the tracker's "Polly bulkhead-depth column" doc; DriverResilienceOptions carries BulkheadMaxConcurrent/BulkheadMaxQueue defaults (DriverResilienceOptions.cs:23–25); the parser merges operator overrides for both (DriverResilienceOptionsParser.cs:116–117); and the AdminUI form authors them (ResilienceFormModel.cs:18–20). But DriverResiliencePipelineBuilder.Build (DriverResiliencePipelineBuilder.cs:99–177) adds only timeout → retry → circuit-breaker — there is no AddConcurrencyLimiter (or any rate/concurrency strategy) anywhere in src/. The bulkhead is a dead knob end-to-end: an operator can set it, the deploy respawns the driver, and nothing changes — the same "configured but inert" genre as the dispatch gap this cycle just fixed, now one layer deeper.

Recommendation: either add the concurrency limiter to the pipeline (keyed like the breaker, per host) with a factory test proving the queue bound engages, or strip the fields from the options/parser/form and the word "bulkhead" from the docs until the strategy ships. Don't leave the knob authored-but-unwired — this codebase's history says it will be trusted.

---

Maturity Ratings

Dimension	2026-07-08	2026-07-12	Justification
Stability	4	4	The deliberate failure-mode engineering (fail-closed caches, detached hooks, null-object seams) still defines the domain, and the new dispatch wiring was built carefully (respawn-on-change, invalidate pairing, actor-context preservation). But nothing on the stability backlog moved — S-1's swallow-everything deploy reporting remains the headline — and the new machinery introduced a genuine operator-reachable bricking vector (S-6) that directly contradicts its own never-brick contract.
Performance	3	3	All four performance findings are untouched (4× JSON parse, per-node authorization garbage, task-per-subscription engine, per-write allocation). The new code is hot-path conscious (once-per-Create options snapshot, callback-free pipelines without observers) but adds no throughput improvements to the existing paths.
Conventions	4	4	Already exceptionally consistent; this window added mechanical enforcement (tree-wide OTOPCUA0001 as a build error, the reflection forwarding guard) which is the right direction — but the systemic byte-parity duplication (C-1/C-3) is untouched, the two flagged stale class narratives survived a dedicated fixdocs pass (C-7), and the new guard itself has a namespace blind spot (C-8).
Underdeveloped areas	3	3	The cycle's biggest dormancy (CapabilityInvoker never dispatched) is genuinely fixed and analyzer-guarded, and U-3 turned out mostly mechanically closed. But U-1's three dead code paths and U-2's implementation-less Tier C machinery are untouched — the latter now accreting config/validation surface — and a new authored-but-inert knob shipped (U-6 bulkhead). The pattern the review flagged (built, tested, unreachable) is still being produced.