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