The issue reports a missing feature. The cause is a stale projection, and the
consequence was worse than the gap described.
`ScriptTagCatalog` still emitted `Tag.TagConfig.FullName` — the pre-v3 driver wire
address. Since v3 the mux key is the **RawPath**: `AddressSpaceComposer` harvests the
`ctx.GetTag("…")` literals into `DependencyRefs`, those become `VirtualTagActor`
dependency refs registered with `DependencyMuxActor`, whose `_byRef` is keyed Ordinal
by `AttributeValuePublished.FullReference` — and a driver's wire ref IS the RawPath
(`DriverHostActor._nodeIdByDriverRef` is keyed `(DriverInstanceId, RawPath)`). The
`{{equip}}/<RefName>` form already substitutes to RawPaths at both compose seams.
So the catalog was wrong in BOTH directions, not merely incomplete: completion
offered paths that could never resolve at runtime, while a CORRECT absolute RawPath
got no completion and hovered as "not a known configured tag path". The file's own
comment admitted the raw/UNS catalog was left for "Batch 2/3" and it never came.
Now projected through the shared `RawPathResolver` — the same byte-parity authority
`DraftValidator` and `AddressSpaceComposer` use — so a suggested path is identical to
the one the deploy gate and the runtime compute. A tag with a broken ancestry chain
resolves to null and is omitted: the runtime could not route it either, so offering
it would be a lie. Hover additionally gains the owning `DriverInstanceId`, which the
topology now makes available (it was always null before).
Editor-accepts <=> publish-accepts is preserved: no diagnostic keys off this catalog
(`OTSCRIPT_EQUIPREF` only marks `{{equip}}` paths), so this changes completion and
hover only — it cannot newly reject a script.
The existing tests pinned the v2 contract and were re-authored: a RawPath only exists
if the RawFolder -> DriverInstance -> Device -> TagGroup -> Tag chain does, so the
seed now builds one. Added coverage for group-nested paths, a driver at the cluster
root (no folder segment), a broken chain being omitted rather than guessed, and the
pre-v3 FullName no longer resolving.
Live-gated on docker-dev (rig rebuilt, both central nodes):
- completion, empty literal -> every RawPath incl. group-nested
`opcua1/plc/OpcPlc/Telemetry/Basic/AlternatingBoolean`
- completion, prefix `pymod` -> `pymodbus/plc/HR200X`, `pymodbus/plc/ImportedTag`
- hover `pymodbus/plc/HR200X` -> "Tag · Type UInt16 · Driver MAIN-modbus"
- hover an unknown path -> "Not a known configured tag path"
AdminUI 742/742; script-analysis 55/55; solution builds 0 errors.
Wave 1 of the driver-expansion program. The branch was complete and live-gated but
had never been merged; it sat 15 commits ahead and 50 behind master.
Purely additive behind the existing IModbusTransport seam — the application protocol
(function codes, codecs, planner, coalescing, write path, probe, materialisation,
HistoryRead) is identical across TCP and RTU; only the wire framing differs
([addr][PDU][CRC-16], no MBAP, no TxId). Zero changes to codecs/planner/health.
Selected by a new Transport config field (ModbusTransportMode: Tcp | RtuOverTcp),
routed through ModbusTransportFactory (throws on unknown mode), with the string-enum
guard on options/DTO/factory and a Transport selector on the AdminUI Modbus form.
ModbusSocketLifecycle was extracted from ModbusTcpTransport (behaviour-preserving)
and is composed by both transports.
Descoped by design: direct-serial (System.IO.Ports) and Modbus ASCII — RTU buses are
reached exclusively via a serial->Ethernet gateway. No per-tag config changes; the
existing per-tag UnitId already makes a multi-drop bus work.
Re-validated against current master at merge time (the branch's own gate predates 50
commits of master): no overlapping files, clean merge, solution builds 0 errors, and
Modbus 344/344, Modbus.Addressing 161/161, AdminUI 740/740, Runtime 507/507,
Configuration 131/131 all pass. Confirmed the Transport selector is still reachable
through master's current authoring path (DriverConfigModal -> ModbusDriverForm), so
the branch's live /run gate result still applies.
Option A from the issue — the cheap consistency check, no interface change.
The desired ref set was re-applied on a Connected TRANSITION (`ResubscribeDesired`)
and on a deploy's `SetDesiredSubscriptions`, and nowhere else. A subscription lost
while the driver STAYED Connected — a subscribe that failed and was never retried,
a handle dropped down an error path — left the actor subscribed to nothing while
still reporting Healthy, indefinitely.
`ReconcileSubscription` now rides the existing 30 s `health-poll` tick: while
Connected, a driver holding a desired set but no live handle is re-subscribed.
Gated on `ISubscribable`. A driver that cannot subscribe at all can still be handed
a desired set, and without the guard it would be told `Subscribe` every tick and
fail every tick, forever.
This is a state-machine consistency check, NOT a probe — it can only see that this
actor holds no handle, never that a handle is present but dead server-side, because
`ISubscriptionHandle` is opaque. That is option B, and it stays deferred until a
real occurrence shows the handle outliving the subscription; it would need a
liveness member implemented across all eight drivers with different subscription
mechanics.
A redundant `Subscribe` is possible (a tick queued ahead of an already-queued one
observes the same no-handle state) and is harmless: `HandleSubscribeAsync` is a
`ReceiveAsync`, so no tick is processed mid-subscribe, and it drops any prior
subscription before establishing the new one. Suppressing it would need a
pending-subscribe flag threaded through every self-tell site — more state than the
race is worth.
`healthPollInterval` becomes an optional Props parameter, mirroring the existing
`rediscoverInterval` seam, so a test can drive the reconcile without waiting 30 s.
Three tests. The reconcile itself is proven by a stub whose FIRST subscribe throws:
nothing else would ever retry it, so a second attempt can only come from the
reconcile — disabling `ReconcileSubscription` turns it red. The two guard tests are
absence assertions and are bounded by a positive control rather than a sleep: a
counting health publisher makes ticks PROCESSED observable, so "still one subscribe"
means the reconcile ran and declined, not that the timer had not fired.
Explicitly not deploy-time drift (#486) — re-asserting an already-correct desired
set would not have helped there.
`ClusterNode.DriverConfigOverridesJson` is the third surface a Sql driver's config
is persisted on, and the only one nothing gated. It is a map keyed by
`DriverInstanceId` merged onto the cluster-level `DriverConfig`, so a literal
`connectionString` pasted there leaks exactly as one pasted into the driver — the
leak #498 closed for `DriverInstance.DriverConfig` and `Device.DeviceConfig`.
Gated at the SAVE, not at the deploy — a deliberate departure from both options the
issue offered:
- `DraftSnapshot` carries no `ClusterNode` rows, and adding them would widen the
snapshot and every builder of one for a single rule about a value that never
enters the artifact.
- More to the point, a deploy gate is the wrong instrument here. Node overrides are
not in the artifact, so blocking a deploy would not stop the credential being
stored — it is already in the database and replicated by then. Refusing the save
is the only point where "refuse to store it" is literally true, which is #498's
own framing: discarding a secret on read is not the same as refusing to store it.
The check moves into a shared `SqlCredentialGuard` so the two enforcement points
cannot drift; `DraftValidator` now calls it instead of its own private helper.
Kept deliberately narrow — the `Sql` driver's `connectionString` only, and only for
instance ids that ARE Sql drivers. The issue asked whether to generalise to
credential-shaped keys across every driver type; not doing that, for the same
reason #498 did not: a broader sweep would start refusing configs that are
legitimate today for drivers which never made an indirect-credential guarantee,
which is a regression rather than defence in depth. Widen per driver, as each gains
its own contract.
The error names the offending driver instance(s) and NEVER the value — it reaches
the AdminUI and the audit trail.
14 new cases cover both halves, including the case variants (System.Text.Json binds
`ConnectionString` to `connectionString`, so a case variant is the same key, not a
bypass), non-Sql ids being ignored, and every blank/malformed/non-object shape.
`AwaitAssert` returns on its FIRST success, so an assertion that is already true at
t=0 passes instantly and spends none of its duration. All three sites asserted an
absence against a collection that starts empty, so none of them ever gave the
handler the time their comments claimed.
Replaced with mailbox-ordering positive controls rather than sleeps:
- `RouteNativeAlarmAck_unknown_node_is_dropped` — follow the unmapped ack with a
MAPPED one. The host forwards via `Tell` and the child handles `RouteAlarmAck`
with `ReceiveAsync` (which suspends its own mailbox), so once the control reaches
the driver the unmapped one has provably been handled.
- `RouteNativeAlarmAck_on_non_primary_is_dropped` — the role itself is the control:
return the node to Primary and re-send. Comments (`gated` / `control`) are the
discriminator, so a leaked ack is visible rather than merged into a count.
- `PeerProbeSupervisorTests.Single_node_snapshot_spawns_no_children` — a FOURTH site
the issue did not list. It says the other `ChildCount.ShouldBe(0)` waits are each
preceded by a `ShouldBe(1)`; that is true of three of the four, but not this one,
which asserts the initial state. Now followed by a two-node snapshot: the count
reaching 1 proves the single-node snapshot was processed, and a local-node child
would have made it 2.
The issue warned these may turn red, which would be a real defect surfacing. They
did not — the routing is correct. Proven by breaking it deliberately instead:
disabling the Primary gate reddens the Secondary test, and routing unmapped ids to
an arbitrary mapping reddens the unknown-node test. Under the old form both breaks
passed.
Root cause was cross-test measurement leakage, not a timing budget.
EventPump's `Meter` is STATIC — one instance per process — and a `MeterListener`
can only select by *meter* in `InstrumentPublished`. `StartMeterCapture` therefore
received measurements from every `EventPump` alive anywhere in the assembly. xUnit
runs test classes in parallel and several build pumps (`EventPumpStreamFaultTests`,
the `Driver-X` test in this same file), so a sibling's events landed in these
counters. That is why the flake needed a busy box: it needed two tests to overlap.
Measured directly during the gate — `Received = 11` in a run that emitted 10.
The old `Received == Dispatched + Dropped + InFlight` assertion is what turned the
leak into a failure. `InFlight` is DERIVED as `Received - Dispatched - Dropped`, so
the identity is a tautology that cannot fail on a real defect — only on a foreign
increment landing between its four separate `Interlocked.Read` calls. Hence the
reported subject, `counters.Received`. It is deleted, along with the `InFlight`
member, and replaced by direct assertions on the three MEASURED counters.
Three further defects in the same test, all found on the way:
- **The "held" dispatch loop was never held.** `OnDataChange` is `EventHandler<T>`
— void-returning — so `async (_, _) => await gate` is async VOID: it returned to
the dispatch loop at the first await and blocked nothing. Measured `Dispatched =
2..3`, never 0. Drops happened only when the producer happened to outrun the
consumer, so the arrangement the doc comment describes was fiction and the
assertions rode on scheduling luck. Now a synchronous `ManualResetEventSlim.Wait()`
on the loop's own thread, pinned by `Dispatched.ShouldBe(0)`.
- **Fixed `Task.Delay(150)`** replaced by a presence poll on the drop count (#500's
rule). Dropped starts at 0, so it cannot be satisfied by the initial state.
- **Emission is staged** — one event, wait for the dispatcher to enter the handler,
then the other nine — so the arithmetic is exact (`10 - 1 held - 2 buffered = 7`)
rather than scheduler-dependent.
The gate release moved into `finally`, ahead of disposal: `DisposeAsync` awaits the
dispatch loop, which is now genuinely parked on that gate, so a failed assertion
would otherwise hang instead of failing.
Verified: 15/15 clean under the exact contention the issue measured (Runtime +
AdminUI + Core concurrently) — where the half-fixed version still failed on run 4.
Falsifiability: restoring the async-void handler turns the new assertions red.
`Scripts.razor` rendered a hash prefix as `@s.SourceHash[..12]…`. `SourceHash`
is a plain nvarchar(64) with no length floor, so any value shorter than 12
characters threw ArgumentOutOfRangeException out of BuildRenderTree — unhandled
in Blazor, which takes the WHOLE page to an HTTP 500, not just the offending
row. Live-reproduced on docker-dev, where two Script rows carry `h1` and
`h-abs-hr200`: /scripts was a hard 500.
New `Components/Shared/DisplayText.Abbreviate(string?, int)`:
- null/blank renders the Admin UI's em-dash placeholder rather than throwing;
- a value already within budget renders verbatim with NO ellipsis, so the
display never implies text that isn't there (the old markup appended "…"
unconditionally, outside the slice — `h1` would have shown as `h1…`);
- a maxLength < 1 is clamped, so a display bug cannot become a page crash.
Applied to every unguarded range-operator slice over a DB-sourced string found
by sweeping the AdminUI for `[..N]`:
Scripts.razor SourceHash (the reported crash)
Certificates.razor Thumbprint (read off the on-disk store)
Deployments.razor x2 RevisionHash
Clusters/ClusterOverview.razor RevisionHash
The other `[..N]` hits are safe and untouched: Guid.ToString("N") slices
(always 32 chars) and the `Length > 60 ? [..60]` ternaries that self-guard.
NOTE — the issue text blamed the docker-dev seed; that was wrong and is
corrected on #504. `seed-clusters.sql` inserts only ServerCluster, ClusterNode
and LdapGroupRoleMapping. The short-hash rows came from earlier live-gate
authoring. A freshly seeded rig is fine; the trigger is any Script row that did
not go through ScriptEdit / UnsTreeService.HashSource — REST-API authored,
hand-inserted, or migrated in.
Tests: 14 new in DisplayTextTests, covering the short/null/blank/clamped cases
and a never-throws sweep over every value x budget combination. AdminUI suite
739/739.
Live-verified on docker-dev (AdminUI has no bUnit): /scripts 500 -> 200 across
both central nodes, rendering `hash=h1` and `hash=h-abs-hr200` in full;
/deployments still truncates 64-char hashes at 12 with the ellipsis;
/certificates and /clusters/MAIN unchanged.
A deploy that triggers a FULL address-space rebuild clears `_alarmConditions`
and re-materialises every condition node fresh — inactive, acked, confirmed,
unshelved. The engine reloads its persisted state and re-derives Active from the
predicate, so it ends up correct; but there is no transition to report, so
`LoadAsync` yields `EmissionKind.None` and `OnEngineEmission` drops it. Nothing
writes the node.
Live-reproduced on the docker-dev rig against the pre-fix image: an alarm that
had fired and cleared but was still UNACKNOWLEDGED came back from a rebuild
reporting Acknowledged with Retain=false — so it disappeared from
ConditionRefresh entirely, while the engine still held Unacknowledged. An
operator's outstanding alarm silently vanishes from the alarm list on deploy.
`ScriptedAlarmHostActor.ReassertConditionNodes` closes it. After each load it
reads the new `ScriptedAlarmEngine.GetProjections()` and sends one
`AlarmStateUpdate` per alarm NOT in the Part 9 no-event position.
Load-bearing properties:
- Node-only. It sends `AlarmStateUpdate` and nothing else — never the `alerts`
topic, never the telemetry hub. `alerts` is the historization path, so a row
per alarm per deploy would append a duplicate historian/AVEVA record every
time anyone deploys. This is why it reads a projection rather than re-emitting:
`ScriptedAlarmProjection` carries no `EmissionKind`, so there is nothing for
the emission machinery — or a future refactor — to mistake for a transition.
The issue's sketch said `GetState(alarmId)` + `LoadedAlarmIds`; that alone
would have clobbered the node's severity and message, so the projection also
carries the definition's severity, the resolved message template, and the
last-observed worst-input quality.
- Only non-normal alarms. One in the no-event position already matches what the
materialise built, so a never-fired alarm stays byte-identical to a deploy
without this pass (including its Message, which the materialise seeds with the
display name), and an all-normal deploy writes nothing at all.
- Timestamp is the condition's own `LastTransitionUtc`, not the deploy instant.
- No duplicate Part 9 events: `WriteAlarmCondition`'s delta-gate compares against
the node's CURRENT state, so a re-assert onto a freshly materialised node is a
genuine delta (fires once — correct, the rebuild reset what clients see) while
one onto a surgically-preserved node is suppressed.
Tests: 6 new host tests + 6 new engine tests. Falsifiability checked by
disabling the call — 4 of the 6 host tests go red; the other 2 are absence
guards against an over-broad fix and pass either way by design.
Live gate (docker-dev, central-2, pre-fix image vs. rebuilt image):
- pre-fix: condition absent from ConditionRefresh after a rebuild;
- post-fix: present, Unacknowledged, Retain=True, stamped with its own
LastTransitionUtc rather than the restart instant;
- /alerts stayed empty across two re-asserts, with the panel proven live by a
real ACTIVATED transition immediately afterwards — no duplicate history.
The reported symptom was "Runtime.Tests occasionally reports Failed: 1" with no test
name. Instrumenting 30 full-assembly runs reproduced it at 13% (4 runs) and showed it
was never ONE flaky test: five failures across three distinct tests in that batch, and
five more distinct tests over the verification rounds that followed.
Two hypotheses were tested and DISPROVED by measurement before anything was changed:
- Cluster-formation timeout. Every test in this assembly forms a real single-node Akka
cluster over TCP in RuntimeActorTestBase's constructor and waits 5 s for MemberStatus.Up
— 219 formations per run. Instrumented: idle max 960 ms, under-load max 1470 ms, zero
timeouts. A 3.4x margin; not the cause.
- Ephemeral-port exhaustion from that bind churn. TIME_WAIT measured at ~0 throughout.
Not the cause.
FOUR GENUINE ORDERING/LOGIC DEFECTS (none is a timeout):
1. VirtualTagHostActorTests.ApplyVirtualTags_respawns_child_when_plan_changes_in_place
The test loops to accept RegisterInterest/UnregisterInterest in EITHER order — and
then asserts on mux.LastSender, which DEPENDS on that order. Under the [Register,
Unregister] interleaving LastSender is the DYING child and the assertion fails. Now
captures the sender of the Register message as it arrives. Fully deterministic; no
timing involved. (Swept the other four LastSender sites: all drain the Unregister
first, so their ordering is fixed. Only this one was unsafe.)
2. DriverHostActorWriteRoutingTests.Primary_routes_write_via_uns_nodeid_to_driver_by_rawpath
ApplyAck marks the end of the APPLY, not the point a write can succeed: the child is
still in Connecting, which deliberately fast-fails writes ("driver not connected"),
and the NodeId->driver reverse map has not been pushed. Now retries until accepted.
This does not weaken the assertion — every rejection branch replies WITHOUT reaching
the driver, so Writes.Count.ShouldBe(1) still means exactly what it did.
3. HistorianAdapterActorTests.Redundancy_snapshot_without_local_node_...
One 500 ms constant served both presence budgets (AwaitAssert) and absence windows
(ExpectNoMsg). Different quantities that happened to share a number, so the presence
budget could not be raised without slowing every absence check. Split into
AssertTimeout (5 s) and Settle (500 ms).
4. ContinuousHistorizationRecorderTests.Retry_after_writer_failure_eventually_acks
Waited on `outbox == 0`, which is ALSO true before the first append — so the poll
could win the race against the recorder and return having observed the INITIAL state,
after which the CallCount>=2 check outside the block failed against a recorder that
had not run. Both conditions are now polled together with the retry count as the
discriminator. (The preceding test in the same file documents this exact trap.)
THE PRESENCE-BUDGET CLASS:
After those four, three consecutive 30-run rounds each still failed once — on a
DIFFERENT test, in a DIFFERENT file, every time (OpcUaPublishActorApplyFailureTests 2 s,
OpcUaPublishActorRebuildTests 2 s, OpcUaPublishActorTests 500 ms). That is a
distribution, not a defect, and fixing it one test at a time was the wrong method.
RuntimeActorTestBase now carries a documented PresenceBudget (15 s) and 57 sub-3 s
AwaitAssert/AwaitCondition durations across four files route through it.
Why this is safe rather than sloppy: a presence budget is an upper bound before giving
up, not a wait — these helpers poll and return the instant the condition holds. Raising
one costs nothing on the happy path and CANNOT make a genuinely failing assertion pass;
it only changes how quickly a real breakage reports. Absence windows are the opposite
(the elapsed time IS the assertion) and were deliberately left untouched with their own
short, individually-calibrated literals.
ScriptedAlarmHostActorTests is a special case, diagnosed rather than merely raised: its
8 s budget was sized for message passing but actually waits on a REAL Roslyn compile
(ApplyScriptedAlarms -> ScriptedAlarmEngine.LoadAsync compiles every predicate). Cold
script compiles are the slowest and most variable thing in the assembly, so that class
gets 30 s with the reason recorded.
Verification: 30 consecutive full-assembly runs, 30 passed / 0 failed, against a 13%
baseline. Full solution builds; all 41 unit-test projects green.
NOT fixed here, filed as #501: DriverHostActorNativeAlarmAckRoutingTests:90 and :116 use
AwaitAssert for an ABSENCE assertion, so they return instantly and prove nothing. They
are excluded from the budget change on purpose — a bigger number does not fix them, and
rewriting them to settle-then-assert may legitimately turn them red.
Until now ISqlDialect.QuoteIdentifier was the SOLE defence for authored
table/column names: an identifier went from the TagConfig blob straight into a
command text, bracket-quoted. The residual risk was bounded — a hostile name is
quoted into one nonexistent object, the query fails after the connection opens,
and the tag Bad-codes — but "bounded" is not "filtered", and the design promised
a filter. Both ISqlDialect and SqlServerDialect carried a doc paragraph saying so.
SqlCatalogGate + SqlCatalogLoader now resolve every authored identifier against
the live catalog at Initialize, and REPLACE it with the catalog's own spelling.
The identifier text in an emitted poll query is therefore a string this driver
read back out of ListSchemas/ListTables/ListColumns — not operator input. Quoting
becomes the backstop it was documented to be.
Decisions worth knowing before touching this:
- Substitution, not just validation. Matching is exact-ordinal first, then a
UNIQUE case-insensitive hit: SQL Server's default collation is CI so case
variants have always worked, and rejecting them would break valid deployments.
An ambiguous CI match under a case-sensitive collation is refused rather than
guessed — picking one would publish another column's data under the operator's
node, which is worse than rejecting the tag.
- Charset check BEFORE catalog lookup. Each identifier goes through
QuoteIdentifier for its rejection rules first, so a name carrying a control or
Unicode format character is rejected WITHOUT its value being echoed into a log
line (Trojan-Source). A name that passes is safe to render, which is why
catalog-miss messages do name it — an operator hunting a typo has to see what
they wrote. Both halves are pinned by tests.
- A rejected tag keeps its node. It is dropped from the POLLED table but stays in
the AUTHORED table, so it still materializes and reads BadNodeIdUnknown —
§8.1's specified outcome. My first wiring dropped it from both, which deleted
the node instead; an existing test (ReinitializeAsync_recoversFromFaulted)
caught it. A status code can only be published by a node that exists, and a
missing address-space entry is far harder to diagnose than a Bad quality.
- An unreadable catalog FAULTS Initialize; it does not reject every tag. That is
the absence of evidence about the tags, not evidence against them — rejecting
all of them would serve a confidently-empty address space and send the operator
hunting typos that do not exist. Zero visible schemas is treated the same way,
because that is exactly what a missing GRANT looks like. Faulting lands
DriverInstanceActor in Reconnecting with its retry timer running.
- Bounded load: one schema list, one default-schema scalar, then one ListTables
per distinct authored schema and one ListColumns per distinct authored relation
— never a full catalog enumeration, and nothing after Initialize. Every
authored name reaches the catalog queries as a bound @schema/@table parameter,
so building the allow-list cannot itself be an injection vector.
- ISqlDialect gains DefaultSchemaSql ("SELECT SCHEMA_NAME()"). An unqualified
`TagValues` must resolve in whatever schema the SERVER reports; hardcoding dbo
would be a silent lie on any estate that maps service accounts to their own
default schema. It is a query, not a constant, because the answer is
per-connection.
- Accepted v1 limitation: a 3-part db.schema.table (or linked-server name)
addresses a catalog this connection cannot enumerate, so it cannot be
allow-listed and is rejected with a message pointing at the fix — expose the
data through a view in the connected database.
VerifyLivenessAsync's wall-clock pattern is extracted to RunBoundedAsync and
reused, so the new I/O gets the same R2-01 / STAB-14 protection rather than a
second hand-rolled copy. (It also fixes a latent bug in the extracted code: the
OperationCanceledException arm detached the wrong task.)
Tests: 24 pure gate tests + 9 end-to-end driver tests against the real SQLite
catalog + 3 new live tests against the real SQL Server on 10.100.0.35 (21 in that
suite now pass, exercising the real SELECT SCHEMA_NAME() + INFORMATION_SCHEMA
path). Verified falsifiable: bypassing the gate turns exactly the three rejection
assertions red and leaves the rest green.
SqlInjectionRegressionTests is deliberately NOT rewritten to expect
BadNodeIdUnknown. It drives SqlPollReader directly, below the gate, and pins the
quoting backstop on its own — defence in depth is only worth the name if each
layer holds independently. Rewriting those assertions would delete the backstop's
only coverage and leave the gate a single point of failure. Its scope note, which
said the gate does not exist, is updated to say why it stays where it is.
The Sql driver's "a pasted literal connection string is never persisted" guarantee was
enforced only on the READ path: SqlDriverConfigDto has no connectionString property and
UnmappedMemberHandling.Skip drops the key on deserialization. Nothing stopped it being
WRITTEN. Config blobs are schemaless JSON columns and the fallback authoring pattern for
a driver without a typed form is a raw-JSON textarea, so an operator pasting
{"connectionString":"Server=...;Password=..."} would put a live credential in the
ConfigDb — persisted, replicated to every node's artifact cache, readable by anyone with
config access — while the runtime silently ignored it and the driver failed to connect.
Discarding a secret on read is not the same as refusing to store it.
DraftValidator.ValidateSqlConnectionStringNotPersisted fails the deploy
(SqlConnectionStringPersisted) when a Sql-typed driver instance's DriverConfig, or the
DeviceConfig of any device beneath it, carries a top-level connectionString key.
- Both surfaces are checked because DeviceConfig is merged onto DriverConfig before the
DTO sees it — a credential pasted into the device blob is the identical leak.
- The key is matched case-insensitively: System.Text.Json binds ConnectionString to a
connectionString property by default, so an ordinal match would leave the obvious
bypass open.
- Only the top level is scanned; the DTO is flat, so a nested occurrence cannot bind and
is not the mistake this rule exists to catch.
- The message never echoes the value — validation errors reach the AdminUI, the deploy
log and the audit trail, and repeating the string there would leak the credential the
rule is refusing to store. Pinned by a test.
- Malformed/blank/non-object JSON never throws: a parse failure would take down every
other rule in the same pass.
16 tests. Verified falsifiable — with the rule disabled the 6 positive assertions fail
and the 10 negatives stay green, so none of them passes vacuously.
Design §8.2's "if an inline connectionString is ever permitted (dev convenience only),
the AdminUI redacts it" carve-out is withdrawn in the same change: redaction only hides a
credential that has already been written.
Not covered, and filed as a follow-up: ClusterNode.DriverConfigOverridesJson is a third
persistence surface for a driver config blob, but it is not part of DraftSnapshot, so
this validator cannot see it.
Every value verified against Opc.Ua.StatusCodes in the pinned SDK (1.5.378.106) by
reflection, not by copying siblings. All are client-visible: OPC UA clients branch on
status.
The three named in #497:
- Historian.Gateway SampleMapper "BadNoData" 0x800E0000 -> 0x809B0000
(0x800E0000 is BadServerHalted)
- Galaxy "BadTimeout" 0x800B0000 -> 0x800A0000
(0x800B0000 is BadServiceUnsupported)
- TwinCAT BadTypeMismatch 0x80730000 -> 0x80740000
(0x80730000 is BadWriteNotSupported)
BadTypeMismatch was wrong in three MORE drivers the issue did not name — FOCAS,
AbLegacy and AbCip carried the same 0x80730000. Every call site is a
FormatException/InvalidCastException conversion failure, so the name was right and
the value was wrong in all four.
Adding the reflection guard then surfaced nine further defects offline tests had
never checked:
- Galaxy GoodLocalOverride 0x00D80000 -> 0x00960000 (not a UA code at all)
- Galaxy UncertainLastUsableValue 0x40A40000 -> 0x40900000
- Galaxy UncertainSensorNotAccurate 0x408D0000 -> 0x40930000
- Galaxy UncertainEngineeringUnitsExceeded 0x408E0000 -> 0x40940000
- Galaxy UncertainSubNormal 0x408F0000 -> 0x40950000
- TwinCAT BadOutOfService 0x80BE0000 -> 0x808D0000 (was BadProtocolVersionUnsupported)
- TwinCAT BadInvalidState 0x80350000 -> 0x80AF0000 (was BadAttributeIdInvalid)
- AbCip/AbLegacy GoodMoreData 0x00A70000 -> 0x00A60000 (was GoodCommunicationEvent)
- Historian.Gateway GatewayQualityMapper Good_LocalOverride 0x00D80000 -> 0x00960000
Galaxy's whole Uncertain block read as though the OPC DA quality byte could be
shifted into the UA substatus position; it cannot — the substatuses are an unrelated
enumeration. GatewayQualityMapper already held the correct table, which is what the
Galaxy values are now reconciled against.
Guard: StatusCodeParityTests (Core.Abstractions.Tests, which already project-
references every driver) reflects over every `const uint` in the deployed
ZB.MOM.WW.OtOpcUa.Driver.*.dll whose name reads as a status code, and asserts it
equals Opc.Ua.StatusCodes.<name>. Discovery is by convention, so a new driver
assembly referenced by that project is covered with no edit here. It went red on all
nine defects above before the fixes and now checks 109 constants green.
Because reflection can only see a NAMED constant, two inline literals were hoisted so
the guard can reach them: Galaxy's BadTimeout/Bad into StatusCodeMap, and
GatewayQualityMapper's 15-entry table into a new HistorianStatusCodes. An inline
`0x800B0000u, // BadTimeout` at a call site is exactly how the Galaxy defect survived.
The drivers stay SDK-free by design; only the test project references Opc.Ua.Core,
as the oracle.
Also corrected: tests that pinned the wrong values (Galaxy StatusCodeMapTests,
SampleMapperTests, GatewayQualityMapperTests — all written from the same bad source),
a mislabelled comment in DriverInstanceActorTests, and stale constants in two design
docs, one of them the unexecuted MTConnect driver design that would have propagated
BadNoData's wrong value into a new driver.
The CLI's SnapshotFormatter value->name table was checked against the SDK and is
correct; no change needed.
The RTU framer overloaded DesyncReason.TruncatedFrame for three distinct
failures — a genuine short read, a CRC-16 validation failure, and a
CRC-valid reply from the wrong RS-485 slave. Split the latter two into
dedicated CrcMismatch / UnitMismatch enum members so a future .Reason
consumer (metrics, operator diagnostics) can tell a wiring/EMI CRC fault
apart from a mis-addressed multi-drop reply. Behaviour is unchanged — all
three still throw ModbusTransportDesyncException and tear the socket down.
Framing tests now assert the specific Reason on each path.
Follow-up #12 from the Modbus RTU-over-TCP plan.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
The read-only Sql driver's runtime factory/probe and tag-side editors were
wired, but the AdminUI could not author the DRIVER: the /raw "New driver" type
list, the DriverConfigModal switch, and the legacy identity dropdown all omitted
Sql, and no SqlDriverForm existed. Live-driving the AdminUI surfaced this.
- Add SqlDriverForm.razor over the SqlDriverConfigDto shape: Provider
(SqlServer-only in v1), required connectionStringRef (an env-var NAME, not a
connection string — label is explicit), optional poll/operation/command
timeouts, maxConcurrentGroups, nullIsBad. Enums serialize by NAME via
JsonStringEnumConverter; a connection-string literal can never be authored
(no such field is collected); rawTags (composer-owned) + allowWrites (inert,
read-only v1) are never emitted.
- Wire Sql into DriverConfigModal switch, RawDriverTypeDialog type list, and
DriverIdentitySection legacy dropdown.
- Add SqlDriverFormContractTests: the exact JSON the form emits constructs a
driver through the real factory, missing connectionStringRef is rejected, and
provider round-trips as the "SqlServer" name (never an ordinal).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
The C1 credential-hygiene fix converted InitializeAsync and ReadAsync to
surface only ex.GetType().Name, but left HandlePollError building its
operator-facing Degrade() message from raw ex.Message, gated only by
'if (ex is DbException) return;'. That guard exists for the I1
double-classification concern, not for message safety, and it is incomplete
for the leak vector: a malformed connection string throws ArgumentException
from the keyword parser (the same unquoted-';'-in-password shape the sibling
SqlDriverBrowser.Sanitize special-cases), which is not a DbException and so
reaches Degrade(ex.Message) unredacted.
It is unreachable in today's control flow only because the connection string
is static and validated identically at Initialize first — an emergent
property, not an enforced invariant, and a live per-poll leak the moment a
future edit (refresh-on-reinit, a different provider) breaks that assumption.
Make the fallback type-only like the other two sites; the full exception still
reaches the log sink via the exception parameter. The I1 defer + control tests
are unaffected (they assert Degraded, not message text). Pinned by a new test
driving a credential-bearing ArgumentException through HandlePollError;
verified load-bearing (red against Degrade(ex.Message), green after).
Closes the C1 review finding on commit 37f444b9.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Add DriverTypeNames.Sql (+ All) as the shared source of truth, and wire the driver
into the Host: register the factory in DriverFactoryBootstrap.Register via
Driver.Sql.SqlDriverFactoryExtensions.Register(registry, loggerFactory), add the
SqlProbe (= Driver.Sql.SqlDriverProbe) probe via TryAddEnumerable, and add the
Driver.Sql project reference to the Host. Default tier A (SQL client is managed +
cross-platform, so ShouldStub needs no change).
Repoint the interim SqlDriver.DriverTypeName / SqlDriverFactoryExtensions.DriverTypeName
literals at DriverTypeNames.Sql; both still resolve to "Sql", so the AdminUI
TagConfigEditorMap/TagConfigValidator keys and the DriverTypeName-parity test stay
green. The Core guard test discovers factories from its own bin, so it also gains a
Driver.Sql project reference — that is the "registered-factory side" that lets
DriverTypeNamesGuardTests' bidirectional-parity check pass (4/4 green).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Review follow-up on the blackhole gate. Two robustness gaps, both bounded to the
disposable dedicated container (never shared infra), fixed:
- If the outer token fired while 'docker pause' was in flight, 'paused' was set
only AFTER the await, so a cancellation there left paused=false while the
container could still complete the pause — the finally then skipped unpause and
leaked a frozen container. Mark paused BEFORE the await, and make the finally's
unpause best-effort (unpausing a never-actually-paused container errors
harmlessly, and a cleanup failure must never mask the real test outcome).
- The pause/unpause shell commands had no wall-clock bound of their own — only the
post-pause read was capped — so a hung SSH handshake would hang CI. Give
RunShellCommandAsync its own 30s hard cap that kills the process and throws
TimeoutException, distinct from an operator's own cancellation.
Offline skip still clean (1 skipped, 9ms, no socket/docker).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
C1: SqlDriver's Initialize/Read failure paths no longer interpolate the ADO.NET
provider's ex.Message into LastError, the thrown message, or host status — only
ex.GetType().Name reaches the operator surface; the full exception goes to the log
sink via the structured logger's exception parameter. The driver is dialect-agnostic
over an arbitrary DbProviderFactory, so it cannot rely on any provider's message
discipline (Microsoft.Data.SqlClient's parser echoes an unrecognised keyword
lower-cased, which a value-based redactor misses).
C1a: replace the vacuous credential test (SQLite's "unable to open" message never
contains the connection string, so it passed regardless) with one that fabricates a
DbException whose own .Message carries a credential token and drives it through the
Initialize liveness-failure path via the injectable factory seam — red against the
pre-fix ex.Message interpolation.
I1: HandlePollError now ignores the DbException class ReadAsync already classified,
so a subscribed-read outage is classified (and logged) once, not twice with the
worse message.
I2: the poll's Healthy verdict routes through a shared SetHealthUnlessFaulted guard
(the one Degrade already used), so a late in-flight poll cannot un-fault a driver a
concurrent ReinitializeAsync just faulted.
I3: DiscoverAsync warns when materializing an omitted-type tag as String, the only
operator signal for the declared-vs-published type mismatch on a numeric column.
M1: BuildTagTable wraps the per-entry parse so a stray non-JsonException throw skips
the tag instead of stranding health at Initializing (it runs before Initialize's
try/catch). M2 left as a documented TODO to avoid touching SqlPollReader.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
SqlTagConfigModel + SqlRowSelectorModel mirror SqlEquipmentTagParser's
accept/reject boundary byte-for-byte on the fields they own: model/type
serialize as NAME strings (never numbers), KeyValue requires
table+keyColumn+keyValue+valueColumn, WideRow requires table+columnName+a
selector (where-pair OR topByTimestamp), Query is rejected, and unknown keys
(top-level and nested rowSelector) survive load->save. Registered in
TagConfigEditorMap + TagConfigValidator keyed off SqlDriver.DriverTypeName
(DriverTypeNames.Sql is deferred to Task 11). A minimal placeholder
SqlTagConfigEditor.razor lands the map entry now; Task 20 fleshes out the UI.
The load-bearing test rounds editor output back through
SqlEquipmentTagParser.TryParse (editor-output <=> parser-input agreement).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Its doc said it pinned "the real provider's cancellation path", but the
token is cancelled before ReadAsync so the OCE is raised at the reader's
SemaphoreSlim.WaitAsync gate, before any SqlConnection opens — it never
exercises Microsoft.Data.SqlClient's in-flight command cancellation.
Softened (option (a)) to state accurately what it proves: an already-
cancelled token propagates as OCE and is never swallowed into a Bad
snapshot or an unreachable-database verdict. The in-flight/deadline path
is covered by SqlBlackholeTimeoutTests.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Adds the frozen-peer / BadTimeout gate for the Sql driver — the single
highest-value integration test. Mirrors the S7 R2-01 blackhole gate:
docker pause a DEDICATED mssql mid-poll, assert the next read surfaces
BadTimeout within the client-side operationTimeout (≈3s) and well below
the server-side CommandTimeout backstop (30s), the driver degrades
(Degraded + host Stopped), and docker unpause recovers to Healthy/Running.
- Docker/docker-compose.yml: disposable `otopcua-sql-blackhole` mssql on
:14333 (never the shared :14330 ConfigDb server) + one-shot seed.
- Docker/seed.sql: the two sample tables.
- SqlBlackholeTimeoutTests.cs: env-gated (SQL_BLACKHOLE_ENDPOINT); pauses
ONLY the hard-coded container name; skips loudly if the endpoint is the
shared port 14330; bounds its own wait so a wedged impl fails, not hangs.
Offline: clean skip (no socket, no docker shell-out).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Locks the driver's injection guarantee against the SQLite fixture. An
authored VALUE ('; DROP TABLE TagValues; --) binds as a DbParameter and can
only ever be a key that matches no row (BadNoData); the seed table survives.
A hostile IDENTIFIER in table/column position is dialect-quoted into a
single nonexistent identifier — inert — and the payload never executes.
Scope, stated plainly: design §8.1 also specifies a catalog gate (validate
an authored identifier against INFORMATION_SCHEMA, reject an unknown one as
BadNodeIdUnknown). That gate does NOT exist in the driver yet and no task
here builds it, so a hostile identifier is not rejected up front — it is
quoted, the query fails after the connection opened, and the tag Bad-codes
as a query failure (BadCommunicationError), not BadNodeIdUnknown. These
tests assert what the code actually guarantees — the payload is inert and
the table intact — rather than a catalog gate that isn't there. The gate is
a tracked follow-up.
No implementation change: the reader already binds correctly (Task 7); this
suite pins it. Falsifiability-checked — forcing a real DROP before the
row-count assertion turns it red, so "table survives" is load-bearing.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
The AdminUI "Test Connect" probe for the Sql driver: open a connection, run
the dialect's LivenessSql (SELECT 1) under a linked-CTS deadline, return
green with latency or red with a reason. Implements IDriverProbe; never
throws (malformed JSON, missing connectionStringRef, unprovisioned
connection string, a provider open failure, timeout, cancellation all
become red results).
Parses the SAME factory DTO with the SAME JsonStringEnumConverter as
SqlDriverFactoryExtensions (R2-11 factory parity, mirroring
ModbusDriverProbe) and resolves connectionStringRef the same way, so a
config that Test-Connects is the config that Deploys.
Credential hygiene: the resolved connection string never reaches the result
message. A provider exception can embed the data source in its OWN message
(the real SqlException shape), so the catch-all names the exception TYPE
only, never ex.Message. The regression test's fake connection embeds the
connection string in its thrown message specifically so surfacing ex.Message
would leak it — verified load-bearing by breaking the guard and watching it
go red.
ForTest injects factory+dialect for the offline SQLite fixture path.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
SqlDriverFactoryExtensions turns a deployed DriverConfig blob into a live
SqlDriver, and SqlConnectionStringResolver resolves the authored
connectionStringRef NAME from Sql__ConnectionStrings__<ref> so credentials
never ride in a config blob (design §8.2).
Three behaviours worth naming:
- String-enum guard. JsonOptions carries JsonStringEnumConverter +
camelCase, so `provider` parses whether authored as a name or an ordinal
and always round-trips as the NAME — the systemic AdminUI defect where a
page serialised an enum numerically against a string-typed DTO.
- Config validation lands here because nothing below does it.
operationTimeout must be STRICTLY greater than commandTimeout (design
§8.3); the reader and the driver stay deliberately usable with the pair
inverted so the frozen-database tests can prove the client-side bound
fires. Non-positive timeouts / poll interval and maxConcurrentGroups < 1
are rejected too.
- Credential hygiene. The resolved connection string reaches the provider
and nothing else: messages name the ref, the environment variable, or
SqlDriver.Endpoint's credential-free server/database rendering. Both a
log-leak and an exception-leak test cover it.
DTO changes:
- Adds RawTags (List<RawTagEntry>), following the Modbus precedent — the
deploy artifact delivers a driver's tags this way and the DTO had no way
to receive them, so an authored Sql tag could never reach the driver.
- Deletes SqlProbeDto + the `probe` key. It had no consumer: SqlDriver was
specified without a background probe loop, and deliberately so — its
IHostConnectivityProbe state is a by-product of the Initialize liveness
check and of every poll, i.e. a statement about traffic that actually
happened rather than a synthetic ping. On-demand connectivity is Task
10's SqlDriverProbe. Shipping a config key that silently does nothing is
worse than not shipping it; UnmappedMemberHandling.Skip means a blob that
still carries `probe` keeps parsing.
allowWrites stays inert (SqlDriver implements no write capability) but an
authored `true` now WARNS rather than passing silently — the flag cannot do
harm, an operator who believes writes are enabled can.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Opens one transient DbConnection from a form-supplied driver-config blob and
hands ownership to SqlBrowseSession, which closes it on disposal. A database is
named either by connectionStringRef — resolved in the AdminUI process from
Sql__ConnectionStrings__<ref>, with an unresolvable ref naming the exact missing
variable — or by a pasted, session-only literal. The literal wins and the ref is
then not read; it cannot arrive from a persisted blob (SqlDriverConfigDto has no
such property), so its presence proves an operator typed it now.
Credential hygiene is the point of the type: no cached config field, nothing
persisted, and no connection text in any log line or exception. Live-probed:
Microsoft.Data.SqlClient and Microsoft.Data.Sqlite keep connection-string values
out of SqlException/SqliteException, but their connection-string PARSER echoes an
unrecognised keyword verbatim — and an unquoted ';' inside a password splits, so
the tail of the password is reported as a keyword (Password=Sup3r;SecretTail =>
"Keyword not supported: 'secrettail;connect timeout'."). The parser's message is
therefore never surfaced; every other failure additionally passes through a
substring redactor that drops the inner exception when it fires.
DriverType comes from SqlDriver.DriverTypeName, the driver's interim local
constant — DriverTypeNames.Sql is added by the driver-factory task.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Three review findings against SqlPollReader (967e5140).
1. WideRow's where-pair form emits no ORDER BY and no row limit, so a
selector that is not actually unique made the reader publish an
arbitrary, storage-order-dependent row — silently, unlike IndexByKey's
duplicate-key warning. Row selection moves out of MapMember into a new
SelectWideRow, which is called once per group and emits the same
rate-limited contract warning (naming table, selector and row count).
The row-limit half of the suggested fix was deliberately NOT taken: a
TOP 1 with no ORDER BY is not deterministic either, and it would
destroy the Rows.Count > 1 evidence the warning depends on, turning a
loud misconfiguration into a permanently silent one. Rationale is in
SelectWideRow's docs. No SQL changed, so no planner golden string moved.
2. The "a frozen database can never accumulate more than
maxConcurrentGroups connections" claim was asserted in the docs and
untested. ReadAsync_aTimedOutGroupKeepsItsConcurrencySlotUntilTheQuery-
TrulyFinishes wedges a group behind an EXCLUSIVE transaction, times it
out, and proves the slot is still held (a second poll on a cap of 1
times out on the gate without ever reaching the factory) and is handed
back only once the abandoned query truly finishes. Moving the release
from the work to the waiter fails this test and no other.
3. Task.Run thread-pool starvation: documented, not changed.
Microsoft.Data.SqlClient's async path parks no pool thread on a frozen
server, so the driver cannot starve itself; LongRunning would cost a
dedicated OS thread per group per poll forever to insure against a
provider this driver does not ship. RunGroupAsync now records the
decision and what a BadTimeout does and does not mean, for the
blackhole gate.
Also: the timeout warning now names the group's table, so a BadTimeout
storm identifies which source is frozen.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Walks schemas -> tables/views -> columns via ISqlDialect's catalog SQL, with
@schema/@table bound as parameters at every level. Column leaves carry the
dialect-mapped DriverDataType in the attribute side-panel (ViewOnly, read-only
v1).
The NodeId encoding deliberately departs from the design sketch's literal
`schema.table|column`: SQL Server permits `.` and `|` inside a quoted
identifier, so that form mis-parses (main.a.b|c reads equally as schema
`main`+table `a.b` and schema `main.a`+table `b`) and silently binds an
operator's tag to the wrong column. SqlBrowseNodeId encodes
`<kind>:<part>[|<part>...]` with `\`/`|` escaped and the kind prefix carrying
the arity; it is public because the picker body decodes it back.
The session owns the connection it is handed and closes it on dispose -- the
registry-held session is the only lifetime hook, so a non-owning session would
leak one pooled connection per reaped picker. Per-call work stays bounded by
the AdminUI's existing 20s linked CTS (BrowserSessionService.PerCallTimeout);
no second deadline is invented here.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
SqlDriver implements IDriver / ITagDiscovery / IReadable / ISubscribable /
IHostConnectivityProbe / IAsyncDisposable over the landed SqlPollReader,
mirroring ModbusDriver. IWritable is deliberately absent: v1 is read-only
structurally, and every discovered variable is SecurityClass=ViewOnly.
The shell classifies poll outcomes because nothing below it does. The reader
honours IReadable literally — it throws only when the database is unreachable
and Bad-codes everything else — so a frozen database returns all-BadTimeout
snapshots through a perfectly successful PollGroupEngine tick. Without
ObservePollOutcome the driver would report Healthy while every value was Bad.
Connection-class codes (BadTimeout / BadCommunicationError) degrade health and
report the host Stopped; authoring-class codes (unresolvable RawPath, absent
row, type mismatch) change nothing, so a tag typo never reports the database
down. It deliberately does NOT synthesise an exception to earn engine backoff:
the engine's exception path publishes nothing, which would cost clients the
Bad quality the reader went out of its way to produce.
Initialize builds the authored RawPath table first (pure, cannot fail; a
malformed TagConfig is logged and skipped) then verifies liveness over one
open-use-dispose connection bounded by wall clock as well as by token (the
R2-01 lesson) — failure records Faulted and rethrows so DriverInstanceActor
retries. The connection string is never logged: a credential-free
server/database Endpoint is the only rendering that reaches a log, LastError,
or the host status.
DriverTypeNames.Sql is NOT added here — DriverTypeNamesGuardTests asserts
bidirectional parity with registered factories, so the constant must land with
the factory (Task 11). SqlDriver.DriverTypeName carries the string until then.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Code-review Minor: the record typed ParameterNames/Parameters/Members/
SelectedColumns as IReadOnlyList<T> but was handed the planner's live List<T>
instances. SqlQueryPlan documents itself as safe to hold for plan caching, so
an aliased list is a real hazard once the reader starts reusing plans across
polls: IReadOnlyList<T> is downcastable, and one mutation would corrupt every
subsequent poll served by that plan.
Copying at the record boundary fixes it for every call site at once rather
than relying on each planner branch remembering to call AsReadOnly.
Pinned by a test that mutates the caller's lists after construction; verified
load-bearing (reverting the record turns it red).
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Executes a poll pass: resolve refs -> SqlGroupPlanner.Plan -> one command per
group -> slice the result set back to per-tag DataValueSnapshots, positionally
aligned with the caller's reference list (design §3.2).
The frozen-peer contract (design §8.3, the R2-01 S7 lesson) is the core of it.
CommandTimeout is only the server-side backstop; the client-side bound is a
linked CancelAfter PLUS Task.WaitAsync, because a linked token bounds only a
provider that honours it and ADO.NET providers vary. The whole per-group
operation — waiting for a concurrency slot, OpenAsync, and the query — runs off
one operationTimeout budget, so ReadAsync returns on time regardless of what the
database is doing. A breach Bad-codes that group (BadTimeout); caller
cancellation propagates instead, since nobody consumes a torn-down poll.
Also: absent row (BadNoData) stays distinct from a NULL cell (Uncertain, or Bad
under nullIsBad); duplicate plan members are all fed (two tags on one keyValue
bind one parameter but stay two members); the concurrency slot is released by the
work, not the waiter, so a frozen database can never hold more than
maxConcurrentGroups connections; the whole call throws only when the connection
itself cannot open, which is what earns PollGroupEngine's backoff.
SqlStatusCodes is driver-local, matching every sibling driver's own table — there
is no shared helper in Core.Abstractions and drivers do not reference the OPC UA
SDK. Values were read off Opc.Ua.StatusCodes rather than copied from a sibling,
because two of those tables carry transcription errors.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Three findings from the code review of the dialect seam:
- QuoteIdentifier used char.IsControl, which only covers Unicode category Cc.
Bidi overrides, zero-width spaces, soft hyphen and BOM are category Cf and
passed through untouched. They cannot escape the brackets, so this is not an
injection bypass — but the method rejects control characters precisely to
protect the integrity of logged/audited statements, and Cf characters spoof
rendered text while comparing byte-different from the real catalog name
(Trojan Source, CVE-2021-42574). Same rule, same rationale, wider category.
- The XML docs on both files asserted that identifiers 'are sourced only from
catalog-validated (INFORMATION_SCHEMA) names' and that rejection is 'a
backstop, not the primary defence'. Neither is true yet: design §8.1 does
specify that gate, but nothing implements it and no task in the plan
schedules one, so an authored TagConfig table/column reaches QuoteIdentifier
unfiltered. The docs now say so, because under-scrutinising this method on
the strength of a filter that does not exist is exactly the failure mode.
- The control-character test only pinned NUL, so a regression to a
Contains('\0') check would still have passed. Pinned the whole Cc category
plus the new Cf rule; verified load-bearing by deleting the guard and
observing exactly the 5 new cases go red.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
The dialect owns the two things ADO.NET's System.Data.Common base types
cannot abstract: identifier quoting and the metadata-catalog SQL. It is
the driver's SQL-injection boundary — values always bind as DbParameters,
identifiers cannot, so the few that reach a command text are catalog-
sourced and pass through QuoteIdentifier.
QuoteIdentifier brackets one identifier part and doubles embedded ] (the
only metacharacter that can terminate a bracketed identifier early);
rejects null/empty/whitespace, >128 chars (T-SQL sysname ceiling), and any
control character (incl. NUL) as ArgumentException, without echoing the
untrusted value into the message. MapColumnType folds the design §3.7
families case-insensitively and never throws — an unrecognised family
falls back to String so a browse over an exotic column still renders;
timestamp/rowversion and time are deliberately NOT mapped to DateTime.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
Joseph Doherty