test+docs: retire the stale EquipmentTagsDarkBatch4 skips, guard the deploy-time

tag gate, and fix three silently-broken OpcUaClient integration tests

Continues working through deferment.md's remaining items.

SKIPPED TESTS (13 -> 0 for this reason). The skip said "dark until Batch 4";
Batch 4 shipped as v3.0 and RETIRED the equipment-tag path rather than lighting
it, so every one of them was waiting on something that had already happened and
gone the other way. Resolved individually rather than in bulk:
  - REVIVED onto the raw/UNS shape (6): four DriverHostActor primary-gate cases,
    the cluster-scoping rebuild, and the real-SDK dual-namespace materialisation
    E2E. The behaviour never went dark, only the v2 seeding did.
  - FOLDED into a live parity test (3): DeploymentArtifactRawUnsParityTests
    already compared RawTagPlan record-equal and RawTagPlan carries array /
    historize / alarm intent, so widening its corpus by two tags covers what
    three empty placeholder suites had been promising. Those suites are deleted.
  - DELETED (4): subjects genuinely retired -- equipment-namespace EquipmentTags,
    and both dot-joint {{equip}}.X token suites (that resolver was deleted in
    Batch 3; the slash-joint form is covered by DeploymentArtifactEquipRefParityTests).
Falsified, not assumed: removing the seeded UnsTagReference turns two revived
primary-gate tests red; re-homing the SITE-A node to MAIN turns the scoping test
red. Widening a parity corpus also needed direct value assertions -- parity alone
cannot distinguish "both seams right" from "both seams blind".
Runtime.Tests skips 13->3, OpcUaServer.Tests 4->1; all remaining are deliberate
EquipmentDeviceBindingRetired tombstones.

G-7 (deploy-time TagConfig gate). MQTT shipped an Inspect() nobody ever called;
now wired. Replaced the hand-maintained list with a reflection guard, because the
existing test enumerates driver types by hand and so guards renames but can never
notice a gap. NOTE: the first version of that guard was hollow -- it discovered
candidates via GetReferencedAssemblies(), which is circular, since the compiler
elides a reference nothing in the IL uses. Removing MQTT from the map also removed
its Contracts assembly from the manifest, and the guard passed green with the bug
reintroduced. Rewritten to scan the output directory; re-falsified and it now
fails naming MqttTagDefinitionFactory. Residual recorded at the code: Sql,
MTConnect, Calculation and OpcUaClient have no Contracts-side Inspect at all, so
the deploy gate stays weaker than the AdminUI's authoring gate for them.

OPCUACLIENT INTEGRATION TESTS (3 of 4 failing, on master too). Not fixture rot:
the simulator was healthy and Client.CLI read the very same node Good. v3 made the
driver resolve every reference through its authored RawTags table, so a bare
"ns=3;s=StepUp" fails LOCALLY at the resolver with BadNodeIdInvalid and never
reaches the wire -- the tests were exercising the unresolved-reference guard.
Fixed by seeding OpcPlcProfile.RawTags in the deploy artifact's TagConfig shape
and addressing by RawPath. 4/4 pass.

DOCS. Applied the Phase7* -> AddressSpace* rename across the four live docs
(historical bannered files deliberately untouched), resolving the three that are
not renames to their real members. Found en route that the earlier banner pass
missed three files: VirtualTags.md, OpcUaServer.md and ScriptedAlarms.md all
still presented the test-scaffolding GenericDriverNodeManager as the production
dispatch path. All three now carry the correction.

Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
2026-07-28 14:32:13 -04:00
parent 5fff597324
commit 4624141a5d
24 changed files with 556 additions and 495 deletions
+134 -5
View File
@@ -47,8 +47,8 @@ themes into tracked decisions. What remains:
| **#521** `IHostConnectivityProbe` / `DriverHostStatus` dead surface | Decision: build the publisher or delete it. The table was deliberately re-created in the v3 initial migration, so deleting on inference would be wrong. | §3.2 |
| **#522** driver stability tiers inert | Decision: pass real tiers or delete the machinery. Docs now say every driver runs Tier A. | §3.3 |
| ~~**#523** remove the periodic 5-min device re-browse~~ | ✅ **Done** — removed. **AbCip and FOCAS now have no tag-change signal at all**; that is the accepted consequence, not an oversight. | §9 (2026-07-28, #523) |
| **G-7** `EquipmentTagConfigInspector` covers 6 of 12 driver types | Pre-existing gap, not a regression; the only §1.2 gap left. | §1.2 |
| **13 `EquipmentTagsDarkBatch4` skipped tests** | Still stale — their unblock condition already happened and the path was retired instead. | §4.4 |
| **G-7** `EquipmentTagConfigInspector` coverage | ◐ **Narrowed + guarded** (2026-07-28): MQTT wired (it shipped an `Inspect` nobody called), and a reflection guard now fails the build if a reachable driver ships one and is not mapped. **Residual:** Sql / MTConnect / Calculation / OpcUaClient have no Contracts-side `Inspect` at all, so the deploy gate stays blind for them while the AdminUI validates them — see §1.2. | §1.2 |
| ~~**13 `EquipmentTagsDarkBatch4` skipped tests**~~ | ✅ **Done** — 6 revived onto the raw/UNS shape, 3 folded into a widened live parity corpus, 4 deleted as retired. The reason constant is gone. **Skips now: `Runtime.Tests` 13 → 3, `OpcUaServer.Tests` 4 → 1** — all remaining are deliberate `EquipmentDeviceBindingRetired` tombstones. | §9 (2026-07-28, skips) |
| 17 other tracker issues | Driver features + deferred work, unchanged by this remediation. | §2 |
| Open live gates | #491 continuous historization, #468 browser tree, archreview R2-03 / R2-10. *(The un-gated drift detector's gate closed by deletion — see #523.)* | §5 |
@@ -99,7 +99,7 @@ Every gap below is in the **authoring/dispatch layer, not driver runtime code**.
| G-4 | **No parity test guards `DriverConfigModal` or `DeviceModal`**`RawDriverTypeDialogCoverageTests`/`…ParityTests` assert `DriverTypeNames`**picker** parity only | — | **This is exactly why G-1 and G-2 survived review.** The guard pattern exists and simply was not extended to the two downstream dispatch maps. | ✅ Fixed — `DriverFormMapParityTests` + `DriverDispatchMapParityTests` |
| G-5 | **`CsvColumnMap` has no typed columns for Sql, MQTT, MTConnect** | `CsvColumnMap.cs:340-452` | CSV import/export degrades to the raw `TagConfigJson` fallback while the 7 older drivers get typed columns. | ✅ Fixed — CSV maps for Sql/Mqtt/MTConnect |
| G-6 | **`RawBrowseCommitMapper` has no Sql branch** — falls to the generic `WriteSingleKey("address", …)` whose comment claims "Browsable drivers are all handled above" | `RawBrowseCommitMapper.cs:121-141`, fallback `:145` | Untrue since `SqlDriverBrowser` was registered (`EndpointRouteBuilderExtensions.cs:83`). Same failure mode the MTConnect branch (`:135-139`) was added to avoid: typed editor opens empty, blanks the field on save. | ✅ Fixed — **and at the browser end too**, which the audit missed |
| G-7 | **`EquipmentTagConfigInspector` covers only 6 driver types** | `:24-29` | Pre-existing (OpcUaClient + Galaxy absent too); no new driver was added. | ⏳ **Open** — pre-existing, unchanged |
| G-7 | **`EquipmentTagConfigInspector` covers only 6 driver types** | `:24-29` | Pre-existing (OpcUaClient + Galaxy absent too); no new driver was added. **The audit understated it:** the AdminUI's `TagConfigValidator` covers **11 of 12**, so the *deploy* gate is weaker than the *authoring* gate — and config arriving by CSV import or API only meets the deploy gate. | ◐ **Narrowed + guarded** — MQTT wired, reflection guard added; 4 drivers still have no Contracts-side `Inspect`. See §9 |
### 1.3 Stale in-code comments (harmless at runtime, misleading to the next reader)
@@ -283,8 +283,8 @@ commented-out DI registrations.** Real deferred work lives in prose doc-comments
| Count | Reason | Assessment |
|---|---|---|
| ~~18~~ 0 | ~~`DiscoveryInjectionDormantV3`~~ | ✅ **Deleted** (`adce27e7`). The audit's "Real — blocked on #507" was **wrong**: these were v2 characterization tests asserting an equipment-rooted graft (`EquipmentRootNodeId == "EQ-1"`) that v3 cannot produce, so they could never have unskipped as written. `Runtime.Tests` skipped went 31 → 13. |
| 13 | `EquipmentTagsDarkBatch4` | **Still open.** **STALE** — the reason says these unblock "at Batch 4"; Batch 4 shipped as v3.0 and the path was *retired* (`AddressSpaceApplier.cs:415` calls it "the vestigial equipment-tag path"). These will never unskip as written — delete or rewrite the reason. |
| 4 + 1 | `EquipmentDeviceBindingRetired`, `DarkBatch4` | Intentional tombstones preserving retirement rationale — fine as-is |
| ~~13~~ 0 | ~~`EquipmentTagsDarkBatch4`~~ | **Resolved** (2026-07-28). The audit's "STALE" call was right and the constant is now **deleted**. Not by bulk-unskipping: 6 were **revived** onto the raw/UNS shape that replaced the subject, 3 were **folded** into a live parity test whose corpus was widened to cover them, 4 were **deleted** as genuinely retired. See §9. |
| 4 | `EquipmentDeviceBindingRetired` | Intentional tombstones preserving retirement rationale — fine as-is |
Plus **4 whole OpcUaClient test suites that are scaffold-only**, awaiting an in-process OPC UA server
fixture that was never built (`OpcUaClientSubscribeAndProbeTests`, `…DiscoveryTests`,
@@ -712,6 +712,135 @@ Build clean; `Runtime.Tests` 476 passed / 13 skipped (unchanged skip set — the
`EquipmentTagsDarkBatch4` group), FOCAS 275, TwinCAT 192, MTConnect 491, AbCip 342,
`Core.Abstractions` 266, all green.
### 2026-07-28 — three silently-broken OpcUaClient integration tests (found by the full-suite run)
Not a register item — found because the full-solution run for the work above showed **three** failing
suites where §9's 2026-07-27 entry had recorded two. `Driver.OpcUaClient.IntegrationTests` was failing 3
of 4.
**It reproduced identically on clean `master`** (verified in a throwaway worktree), so it was not caused
by this work — but it was also not what it first looked like. The chain of wrong guesses is worth
recording, because each one would have closed the investigation early:
1. *"The fixture is down."*`otopcua-opc-plc` was `Up 12 days (unhealthy)`. Restarted it, waited for
healthy. **Still 3/4 failing.**
2. *"The simulator's node set drifted."* — the tests expect `ns=3;s=StepUp`. Browsed the live server:
all three nodes present at exactly those ids. Read `ns=3;s=StepUp` through Client.CLI against the
same endpoint: **`Good`, value 1379.**
3. *"So the driver has a read defect."* — the status code was `2150825984`, which I first decoded by
hand as `BadAttributeIdInvalid` and started hunting a wrong `AttributeId`. **The decode was wrong**
(`0x80330000`, not `0x80350000`) — it is `BadNodeIdInvalid`, and grep found that exact constant on
the driver's own *local* fault path, reached without any wire request.
The actual cause: **v3 made the driver resolve every reference through its authored `RawTags` table**
(RawPath → `TagConfig.nodeId` → live NodeId re-bound against the session's namespace table). A bare
`ns=3;s=StepUp` is no longer a reference the driver accepts. The smoke tests still passed bare node ids
and constructed the driver with an empty `RawTags`, so all three failed locally at the resolver — they
had been testing the unresolved-reference guard, against a perfectly healthy simulator.
Fixed by seeding `OpcPlcProfile.RawTags` with the four nodes in the same `{"nodeId": …}` TagConfig shape
the deploy artifact delivers, and addressing them by RawPath. **4/4 pass.**
Two things this says about the register itself: the 2026-07-27 "only pre-existing environment failures
remain" note was measured against an incomplete baseline (this suite was already broken and simply not
enumerated); and **fixture-gated integration suites rot invisibly** — a driver contract can change under
them and nothing on the local box notices, because the suite is expected to be red-or-skipped depending
on whether a remote container happens to be up.
### 2026-07-28 — §7.2 priority 3: the `Phase7*` → `AddressSpace*` rename
Applied across the four **live** docs (`VirtualTags.md`, `ScriptedAlarms.md`, `ScriptEditor.md`,
`v2/Architecture-v2.md`). The already-bannered historical files (`v2/phase-7-status.md`,
`v2/implementation/*`) were deliberately **left alone** — they are records of what existed at the time,
and renaming inside a historical record would falsify it.
§7.2 was right that three of these are not renames, and each was resolved to the real member:
| Cited | Reality |
|---|---|
| `Phase7CompositionResult` | → **`AddressSpaceComposition`** (and it now also carries the raw subtree, so the doc text was widened, not just renamed) |
| `Phase7Composer.ExtractTagFullName` | → **`ScriptTagCatalog.ExtractFullNameFromTagConfig`** |
| `Phase7EngineComposer.RouteToHistorianAsync` | **Gone, not renamed** — the routing moved into `HistorianAdapterActor` → the registered `IAlarmHistorianSink`. Called out inline so the next reader does not go looking for a renamed symbol. |
**Found en route — the banner pass missed three files.** `GenericDriverNodeManager` is test scaffolding
(§3.4, its own source says "zero production references"), and the earlier remediation bannered
`AddressSpace.md` and `IncrementalSync.md` for it. But `VirtualTags.md`, `OpcUaServer.md` and
`ScriptedAlarms.md` all still presented it as **the** production dispatch path — `VirtualTags.md:3` even
built its opening explanation of driver-vs-virtual dispatch on it. All three now carry the same
correction naming the real chain (`OpcUaPublishActor``SdkAddressSpaceSink``OtOpcUaNodeManager`, two
namespaces). Banners, not rewrites — the underlying rewrite stays owed, consistent with how §7.2 treats
the other two.
### 2026-07-28 — G-7, and what the audit missed about it
The register called this "pre-existing (OpcUaClient + Galaxy absent too); no new driver was added" and
ranked it last. Reading both maps side by side makes it sharper than that:
- The AdminUI's `TagConfigValidator` covers **11 of 12** driver types (all but Galaxy).
- The deploy-time `EquipmentTagConfigInspector` covers **6**.
So the gate that runs at *authoring* is stronger than the gate that runs at *deploy* — and the deploy
gate is the only one config sees when it arrives by **CSV import or API** rather than through the modal.
That is the wrong way round.
**MQTT was a plain hole:** `MqttTagDefinitionFactory.Inspect` has existed since the driver merged and
nothing ever called it. Now wired. The inspector's own doc-comment also claimed "unmapped drivers
(Galaxy, OpcUaClient) are skipped exactly as in the AdminUI validator" — untrue, OpcUaClient *is* mapped
there; corrected.
**A reflection guard replaces the hand-maintained list**, same lesson as G-4: the existing
`EquipmentTagConfigInspectorTests` enumerates driver-type strings by hand, so it guards renames but can
never notice a gap — a newly-inspectable driver simply never appears in it.
⚠️ **The first version of that guard was hollow, and only the falsification step caught it.** It
discovered candidates via `Assembly.GetReferencedAssemblies()`, which is **circular**: the C# compiler
elides a reference nothing in the IL uses, so removing MQTT from the map also removed its Contracts
assembly from ControlPlane's manifest, and the sweep stopped looking for exactly the driver that had just
gone missing. It passed green with the bug reintroduced. Rewritten to scan the output *directory*
(MSBuild copies project-referenced assemblies regardless of IL usage) — re-falsified, and it now fails
naming `MqttTagDefinitionFactory`. It also carries a positive control asserting discovery found more than
one factory, since an empty expected-set is satisfied by an empty map.
**Residual, deliberately not papered over.** Sql, MTConnect, Calculation and OpcUaClient have **no**
`Inspect` in a Contracts assembly — their only checker is the AdminUI `*TagConfigModel.Validate()`, which
lives in the AdminUI project and cannot be referenced from ControlPlane. Closing that means promoting
those models (or an equivalent parser) into each driver's Contracts assembly — a real piece of work, not
a map entry. Recorded in the inspector's own doc-comment so the next reader finds it at the code.
### 2026-07-28 — the 13 stale `EquipmentTagsDarkBatch4` skips
§4.4 flagged these as stale and it was right: the reason said "dark until Batch 4", Batch 4 shipped as
v3.0, and the equipment-tag path was **retired** rather than lit — so every one of them was waiting on
something that had already happened and gone the other way.
**Unskipping them in bulk would have failed, and deleting them in bulk would have thrown away live
coverage.** Each was resolved on its own merits:
| Disposition | Tests | Why |
|---|---|---|
| **Revived** onto the raw/UNS shape | 4 primary-gate cases (`DriverHostActorPrimaryGateTests`), the cluster-scoping rebuild (`OpcUaPublishActorRebuildTests`), the real-SDK materialisation E2E (`AddressSpaceApplierHierarchyTests`) | The *behaviour* never went dark — only the v2 seeding did. Each seeds the v3 chain (RawFolder → Driver → Device → Tag, + `UnsTagReference`) instead. |
| **Folded** into a live parity test | array intent, historize intent, native-alarm/writable intent | `DeploymentArtifactRawUnsParityTests` already compared `RawTagPlan` **record-equal**, and `RawTagPlan` carries all three fields — so widening its corpus by two tags covers what three empty placeholder suites had been promising. Those suites are deleted. |
| **Deleted** | equipment-namespace `EquipmentTags` parse, both `{{equip}}`-token suites | The subjects are retired. The token tests used the **dot-joint** `{{equip}}.X` form, whose resolver (`EquipmentScriptPaths.DeriveEquipmentBase`) was deleted in Batch 3; the slash-joint form is covered live by `DeploymentArtifactEquipRefParityTests` (4 tests). |
Three things worth keeping:
- **The revivals are the point, not the unskipping.** `Unknown_role_single_driver_services_write` asserts
`recorder.Writes.ShouldNotBeEmpty()` — it needs the write to actually reach a driver, which needs a
routable tag. **Verified by deleting the `UnsTagReference` from the seed: it and
`Known_role_wins_over_member_count` go red.** Same check on the cluster-scoping test (re-home the SITE-A
node to MAIN ⇒ red). The three *denial* cases in that file were passing all along — they deny before
routing — which is exactly why the gap was invisible.
- **Widening a parity corpus needed a non-parity assertion to go with it.** A composer that dropped array
intent *entirely* would still satisfy `decoded.RawTags.ShouldBe(composed.RawTags)`, because the artifact
decode would drop it too and the two empties compare equal. Parity cannot distinguish "both right" from
"both blind", so the widened test also asserts the composed values directly.
- **The reason constant is deleted, replaced by a note explaining why.** A skip reason is a claim with an
expiry date, and this one outlived its own condition by a release. Both `DarkAddressSpaceReasons` files
now say so, so the next person writing one states a condition a reader can actually check.
Skipped counts: `Runtime.Tests` **13 → 3**, `OpcUaServer.Tests` **4 → 1**. Every remaining skip is an
`EquipmentDeviceBindingRetired` tombstone — deliberate, and §4.4 already calls those fine as-is.
### 2026-07-27 — §8.4 dispatch-map parity (G-1 … G-6)
**The maps had to become data before they could be guarded.** A Razor `@switch` compiles into