From 6c63bf8fabe318c93a093bed70151b5b67a5abb4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:51:39 -0400 Subject: [PATCH 1/2] fix(mtconnect): back off a /sample re-baseline that resolved nothing; scope Test Connect to the configured device Driver.MTConnect-001 (Medium): StreamVerdict.Reopen reset the reconnect ladder unconditionally. The InvalidDataException catch that routes an Agent OUT_OF_RANGE to a /current re-baseline also catches an unparseable chunk and an over-sized multipart part - neither of which a re-baseline can resolve - so the pump reopened and failed with no delay at any point: two HTTP requests and one Warning per iteration, forever. The Reopen arm now uses the same discriminator the transient arm does (did THIS connection deliver a chunk?), so a buffer roll behind a working stream still reopens instantly while a re-baseline that resolved nothing advances BackoffFor. Driver.MTConnect-002 (Medium): MTConnectDriverProbe requested {AgentUri}/probe while MTConnectAgentClient requests {AgentUri}/{DeviceName}/probe, so a mistyped or since-renamed deviceName showed GREEN on Test Connect and then Faulted the driver at InitializeAsync (an unknown device path answers MTConnectError under HTTP 200). The probe's minimal DTO now reads deviceName - the rule is 'which keys compose the URL', not 'which keys are cheap to read'. Driver.MTConnect-008 (Low): corrected two stale claims that a since-changed RawBrowseCommitMapper invalidated (see Driver.MTConnect-003, which remains open and belongs in AdminUI), and made DiscoverAsync log the options snapshot it deliberately read once instead of the field. Tests: 538 pass (534 base + 4 new, each red before its fix); AdminUI.Tests 1466 pass. --- docs/drivers/MTConnect.md | 18 ++++- .../MTConnectDataTypeInference.cs | 20 ++++-- .../MTConnectDriverOptions.cs | 6 +- .../MTConnectDriver.cs | 54 ++++++++++---- .../MTConnectDriverProbe.cs | 43 ++++++++--- .../MTConnectDriverProbeTests.cs | 59 +++++++++++++++ .../MTConnectSubscribeTests.cs | 72 +++++++++++++++++++ 7 files changed, 239 insertions(+), 33 deletions(-) diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 47e0c7c7..489448ed 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -167,9 +167,11 @@ universal browser commits and the key the driver resolves reads/subscribes against. `MTConnectTagConfigModel.FromJson` (the AdminUI typed editor) tries three spellings in order — `fullName` → `dataItemId` → `address` — and takes the first non-blank one, normalising onto `fullName` on save. `address` is -accepted because that's the field name `RawBrowseCommitMapper` writes for a -driver with no typed editor path, so a browse-committed tag stays readable -even before the editor round-trips it. +accepted because that is the field name `RawBrowseCommitMapper` writes for a +driver with no typed-editor branch of its own — MTConnect has had such a +branch since Gitea #536 and now commits under `fullName`, but blobs committed +before it, and hand-authored/CSV/API blobs, still carry `address` and must +stay readable. ### Coercion-type precedence @@ -192,6 +194,16 @@ into) is resolved in this order, first non-null wins: (category/type/units/representation → `DriverDataType`). 4. `DriverDataType.String` — the coercion that cannot fail. +> ⚠️ **Step 3 does not currently rescue a browse-committed tag.** `RawBrowseCommitMapper`'s +> MTConnect branch emits a whole `MTConnectTagConfigModel`, whose `ToJson()` always writes +> `dataType` from a field defaulting to `String` — a non-blank authored type, so step 1 wins +> and step 3 is never consulted. The raw tag's OPC UA `DataType` meanwhile *is* taken from the +> browsed leaf's inferred type (`RawBrowseCommitMapper.MapLeaf` → `MapDataType`), so a browsed +> `SAMPLE` lands as a `Double` node whose driver coerces the observation to text. Fix belongs in +> the commit mapper (forward the leaf's `DriverDataType` into the model, as the AbCip / TwinCAT / +> FOCAS branches already do). Recorded as `Driver.MTConnect-003` in +> [`code-reviews/Driver.MTConnect/findings.md`](../../code-reviews/Driver.MTConnect/findings.md). + ### Quality mapping `UNAVAILABLE` — MTConnect's one explicit "I have no value" sentinel — maps to diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs index 4bddb20c..d8e16053 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs @@ -29,11 +29,21 @@ public readonly record struct MTConnectInferredType(DriverDataType DataType, boo /// /// /// -/// This lives in .Contracts because three call sites must agree byte-for-byte, or a -/// tag's OPC UA type stops matching its authored config: ITagDiscovery.DiscoverAsync -/// (stamps on every browsed leaf), the -/// universal browser's commit path (writes the browsed leaf into TagConfig), and the -/// AdminUI typed tag editor (shows the inferred type and offers an override). +/// This lives in .Contracts because every call site must agree byte-for-byte, or a +/// tag's OPC UA type stops matching its authored config. At HEAD the callers are both inside +/// the driver: ITagDiscovery.DiscoverAsync (stamps +/// on every browsed leaf) and +/// MTConnectDriver.InferDefinition (the runtime coercion fallback for a raw tag whose +/// blob declares no type). +/// +/// +/// ⚠️ Two call sites this remark used to name do not exist. The universal browser's +/// commit path (RawBrowseCommitMapper) never calls this — it re-maps the browsed leaf's +/// already-inferred DriverDataType onto the raw tag's OPC UA type but writes a +/// TagConfig whose dataType is the model default — and the AdminUI typed editor +/// (MTConnectTagConfigModel) deliberately does not recompute the inference at all. The +/// first of those is a live defect, not merely a stale comment: see +/// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// /// /// MTConnect is weakly typed on the wire — every observation is text, and the standard diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs index 6c601e76..ef6a8edb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs @@ -86,7 +86,11 @@ public sealed class MTConnectDriverOptions /// type comes from the blob's driverDataType; failing that, from a /// entry naming the same DataItem id; failing that, from the Agent's /// own /probe declaration (); failing - /// that, — the coercion that cannot fail. + /// that, — the coercion that cannot fail. + /// ⚠️ The /probe step is not reached by a browse-committed tag at HEAD: + /// the AdminUI's commit mapper writes a full typed blob whose dataType defaults + /// to String, which the first step then honours. See + /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// A tag in only is reachable by its DataItem id /// (the driver CLI's authoring surface, and every pre-v3 test) and still contributes /// its coercion type to the index. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index a666a27d..7734a187 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -1224,7 +1224,7 @@ public sealed class MTConnectDriver // downstream). Its data items are unreachable either way. _logger.LogWarning( "MTConnect driver {DriverInstanceId} skipped a device in {AgentUri}'s /probe model that declares neither a name nor an id; it cannot be given a browse path.", - _driverInstanceId, _options.AgentUri); + _driverInstanceId, options.AgentUri); continue; } @@ -1242,7 +1242,7 @@ public sealed class MTConnectDriver { _logger.LogWarning( "MTConnect driver {DriverInstanceId} is scoped to device '{DeviceScope}', but {AgentUri}'s /probe model declares no device with that name or id ({DeclaredCount} declared). The browse is empty because the scope matched nothing, not because the Agent has nothing.", - _driverInstanceId, deviceScope, _options.AgentUri, model.Devices.Count); + _driverInstanceId, deviceScope, options.AgentUri, model.Devices.Count); return; } @@ -1252,7 +1252,7 @@ public sealed class MTConnectDriver _driverInstanceId, devices, variables, - _options.AgentUri, + options.AgentUri, deviceScope is null or "" ? string.Empty : $" (scoped to '{deviceScope}')"); } @@ -1854,10 +1854,13 @@ public sealed class MTConnectDriver /// "just re-initialize" would be a silent permanent deadlock. /// /// - /// Backoff applies to failures only. A re-baseline is a protocol event, not a - /// fault, so it reopens the stream immediately and resets the attempt count; only a - /// genuine failure (dropped connection, blown heartbeat, a re-baseline that itself - /// failed) advances . + /// Backoff applies to failures only — and to a re-baseline that resolved nothing. + /// A re-baseline behind a stream that delivered is a protocol event, not a fault, so it + /// reopens immediately and resets the attempt count. One on a connection that delivered + /// nothing advances like any other failure, because the + /// path that reaches it also carries conditions a + /// /current cannot resolve (an unparseable chunk, an over-sized part) and would + /// otherwise reopen-and-fail as fast as the Agent can answer. /// /// private async Task RunSampleStreamAsync( @@ -1927,8 +1930,21 @@ public sealed class MTConnectDriver return; case StreamVerdict.Reopen: - attempt = 0; - _reconnectAttempts = 0; + // A re-baseline behind a stream that DELIVERED is the protocol event this + // verdict was written for — a sequence gap or an OUT_OF_RANGE rejection, both + // of which the /current genuinely resolves — so it reopens immediately on a + // clean ladder. + // + // One that delivered NOTHING is not necessarily resolved by anything. The + // InvalidDataException catch that routes OUT_OF_RANGE here also catches an + // unparseable chunk and an over-sized part, and a /current cannot fix either: + // resetting the ladder there reopens instantly, fails identically, and spends + // one /sample AND one /current per iteration forever — the unbounded hot loop + // every other arm of this pump (the Unsupported latch, the backoff floor, the + // non-positive cap) exists to prevent. The discriminator is the same one the + // transient arm below uses, for the same reason. + attempt = outcome.DeliveredAny ? 0 : attempt + 1; + _reconnectAttempts = attempt; break; @@ -3208,10 +3224,22 @@ public sealed class MTConnectDriver /// Coercion type precedence (see ): /// the blob's own driverDataType, else a /// entry naming the same DataItem id, else the Agent's /probe declaration for that - /// data item, else . The third step is what makes a - /// browse-committed tag work: RawBrowseCommitMapper writes only the address into - /// the blob, so without it every value from the address picker would publish as text - /// under a numerically-typed OPC UA node. + /// data item, else . + /// + /// + /// ⚠️ The third step does NOT currently rescue a browse-committed tag, and the + /// AdminUI is where that has to be fixed. It was written on the premise that + /// RawBrowseCommitMapper writes only the address into the blob — true when this + /// was authored, false at HEAD: the MTConnect branch of + /// RawBrowseCommitMapper.BuildTagConfig now emits a whole + /// MTConnectTagConfigModel, whose ToJson unconditionally writes + /// "dataType" from a field defaulting to . That + /// is a non-blank authored type, so step 1 wins and this step is never reached — every + /// browse-committed tag coerces to text while its raw-tag node was typed Double / + /// Int64 from the SAME probe inference (RawBrowseCommitMapper.MapLeaf). + /// The step is kept because it is still the correct rule and still serves a blob authored + /// by hand, by CSV or by API with no type; see + /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). /// /// /// The options document to derive from. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs index 85658b9a..73fe78bb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs @@ -16,19 +16,29 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// Deliberately does NOT delegate to . That /// method builds the full driver options document, including every authored /// MTConnectTagDefinition (BuildTag can throw on a malformed tag entry that has -/// nothing to do with reachability). A probe only needs agentUri, so this type parses -/// its own minimal DTO — a config that is invalid for the driver (bad tag, bad data type) can -/// still be reachability-tested, matching the AdminUI's "does the endpoint answer" intent for -/// Test Connect. It also avoids a compile-time dependency on the factory DTO shape -/// (MTConnectDriverFactoryExtensions), which is authored separately (Task 15). +/// nothing to do with reachability). A probe needs only the fields that compose the request +/// URL, so this type parses its own minimal DTO — a config that is invalid for the driver (bad +/// tag, bad data type) can still be reachability-tested, matching the AdminUI's "does the +/// endpoint answer" intent for Test Connect. It also avoids a compile-time dependency on the +/// factory DTO shape (MTConnectDriverFactoryExtensions), which is authored separately +/// (Task 15). /// /// -/// Accepted gap: reachable ≠ deployable. Because only agentUri is read, a -/// config can show green on Test Connect and still fail to deploy — e.g. a non-positive -/// requestTimeoutMs or a malformed tag entry the factory's full parse would reject. -/// This is the direct consequence of the independent-parsing decision above, judged -/// acceptable because Test Connect's job is "is the endpoint there", not "will this exact -/// config deploy". +/// The minimal DTO is "everything in the URL", not "agentUri". It reads +/// deviceName too, because narrows every read to +/// {AgentUri}/{DeviceName}/probe: probing the agent-wide path instead would answer a +/// different question from the one the operator asked, and a mistyped or since-renamed device +/// would show GREEN here and then Fault the driver at InitializeAsync (the Agent +/// answers an unknown device path with an MTConnectError under HTTP 200). The +/// distinction is not "which keys are cheap to read" but "which keys change the endpoint". +/// +/// +/// Accepted gap: reachable ≠ deployable. Because only the URL-composing fields are +/// read, a config can show green on Test Connect and still fail to deploy — e.g. a +/// non-positive requestTimeoutMs or a malformed tag entry the factory's full parse +/// would reject. This is the direct consequence of the independent-parsing decision above, +/// judged acceptable because Test Connect's job is "is the endpoint there", not "will this +/// exact config deploy". /// /// /// Deliberately DOES reuse and @@ -117,6 +127,12 @@ public sealed class MTConnectDriverProbe : IDriverProbe private sealed class MTConnectProbeConfigDto { public string? AgentUri { get; set; } + + /// + /// The optional device scope. Read because it is part of the URL, not part of the + /// driver's tag/timing configuration — see the "reachable ≠ deployable" remarks on the type. + /// + public string? DeviceName { get; set; } } /// @@ -169,6 +185,11 @@ public sealed class MTConnectDriverProbe : IDriverProbe var options = new MTConnectDriverOptions { AgentUri = agentUri, + + // Part of the request URL, so it must be carried: MTConnectAgentClient narrows every read + // to {AgentUri}/{DeviceName}/probe, and a probe of the agent-wide path answers a different + // question from the one the operator asked. See the type remarks. + DeviceName = string.IsNullOrWhiteSpace(dto.DeviceName) ? null : dto.DeviceName.Trim(), RequestTimeoutMs = requestTimeoutMs, }; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs index 03ce5781..8dd14e9d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectDriverProbeTests.cs @@ -301,6 +301,65 @@ public sealed class MTConnectDriverProbeTests r.Latency.ShouldBeNull(); } + // ── review follow-up: the probe must request the URL the DRIVER will request ── + + /// + /// Test Connect has to probe the driver's own request path, device scope included. + /// MTConnectAgentClient.BuildRequestUri narrows every read to + /// {AgentUri}/{DeviceName}/probe, so a probe that requested the agent-wide + /// {AgentUri}/probe answers a different question from the one the operator asked. A + /// mistyped or since-renamed deviceName then shows GREEN on Test Connect and Faults the + /// driver at InitializeAsync — the Agent answers that path with an MTConnectError + /// under HTTP 200, which the parser surfaces as . + /// + /// This is a different class from the accepted "reachable ≠ deployable" gap the type + /// documents (a bad tag entry, a non-positive timeout): those do not change the URL, and + /// the whole point of a reachability probe is which endpoint it reaches. + /// + /// + [Fact] + public async Task Configured_deviceName_scopes_the_probe_request_the_way_the_driver_does() + { + Uri? requested = null; + var probe = ProbeWithHandler((request, _) => + { + requested = request.RequestUri; + + return Task.FromResult(XmlResponse(ValidProbeXml)); + }); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\",\"deviceName\":\"VMC-1\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeTrue(); + requested.ShouldNotBeNull(); + requested.AbsolutePath.ShouldBe("/VMC-1/probe"); + } + + /// + /// …and with no deviceName the probe stays agent-wide, exactly as the driver does. The + /// falsification control for the test above: unconditionally inserting a segment would pass it + /// while breaking every unscoped config. + /// + [Fact] + public async Task An_absent_deviceName_leaves_the_probe_agent_wide() + { + Uri? requested = null; + var probe = ProbeWithHandler((request, _) => + { + requested = request.RequestUri; + + return Task.FromResult(XmlResponse(ValidProbeXml)); + }); + + var r = await probe.ProbeAsync( + "{\"agentUri\":\"http://stub-agent/\"}", QuickTimeout, CancellationToken.None); + + r.Ok.ShouldBeTrue(); + requested.ShouldNotBeNull(); + requested.AbsolutePath.ShouldBe("/probe"); + } + // ── minor: credentialed agentUri + a SUCCESSFUL response must still redact ── [Fact] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs index b12c32ff..84b5d790 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectSubscribeTests.cs @@ -990,6 +990,78 @@ public sealed class MTConnectSubscribeTests } } + /// + /// A re-baseline that never resolves anything must not reopen without a backoff. + /// StreamVerdict.Reopen is "a protocol event, not a fault", so it reopens immediately + /// and resets the ladder — correct for the two cases it was written for (a sequence gap and an + /// OUT_OF_RANGE rejection), both of which the /current re-baseline genuinely + /// fixes, so they cannot recur. + /// + /// But the catch that routes OUT_OF_RANGE to the + /// re-baseline also catches an unparseable chunk and an oversized part — conditions a + /// /current cannot fix. Reopening those with the ladder reset is an unbounded hot + /// loop against the Agent (one /sample AND one /current per iteration, as fast + /// as they complete, one Warning each), which is exactly what the reconnect backoff exists + /// to prevent everywhere else in this pump. + /// + /// + /// The discriminator is the one the transient path already uses: did THIS connection deliver + /// a chunk? A re-baseline behind a working stream still reopens instantly; one on a + /// connection that has never delivered advances the ladder. Asserted as monotonic growth, + /// never as a duration. + /// + /// + [Fact] + public async Task A_rebaseline_that_delivers_nothing_still_advances_the_reconnect_backoff() + { + // A 1 ms cap keeps the failing loop from actually sleeping; nothing here asserts a duration. + var options = Opts(new MTConnectReconnectOptions { MinBackoffMs = 0, MaxBackoffMs = 1 }); + var (driver, client) = await InitializedDriverAsync(options: options); + + // Sticky, and NOT a transient stream fault: a chunk this driver can never parse. /current keeps + // answering, so every re-baseline "succeeds" and the pump reopens — forever. + client.SampleFailure = new InvalidDataException("chunk is not a well-formed MTConnectStreams document"); + + var handle = await driver.SubscribeAsync(["dev1_pos"], TimeSpan.FromMilliseconds(50), Ct); + + try + { + await client.WaitForSampleCallsAsync(3).WaitAsync(Watchdog, Ct); + var early = driver.ReconnectAttempts; + + await client.WaitForSampleCallsAsync(6).WaitAsync(Watchdog, Ct); + + early.ShouldBeGreaterThanOrEqualTo(2); + driver.ReconnectAttempts.ShouldBeGreaterThan(early); + } + finally + { + await driver.UnsubscribeAsync(handle, Ct).WaitAsync(Watchdog, Ct); + } + } + + /// + /// …and the falsification control for the test above: a re-baseline BEHIND a stream that was + /// delivering keeps its immediate reopen. Without this, "advance the ladder on every re-baseline" + /// would pass the test above while making a healthy Agent's ordinary buffer roll cost a backoff. + /// + [Fact] + public async Task A_rebaseline_after_a_delivered_chunk_still_reopens_immediately() + { + var (driver, client) = await InitializedDriverAsync(); + await driver.SubscribeAsync(["dev1_pos"], TimeSpan.FromMilliseconds(50), Ct); + + // One good chunk, so this connection has provably delivered… + await client.PumpAsync(Chunk(PrimedNextSequence, 113, ("dev1_pos", "1.5"))).WaitAsync(Watchdog, Ct); + + // …then the Agent's buffer rolls past the cursor. The re-baseline is a protocol event behind a + // working stream: no backoff, no ladder. + client.CurrentAnswers.Enqueue(Chunk(5000, 5005, ("dev1_pos", "2.5"))); + await client.PumpAsync(Chunk(5000, 5005, ("dev1_pos", "2.5"))).WaitAsync(Watchdog, Ct); + + driver.ReconnectAttempts.ShouldBe(0); + } + /// /// I3 — a non-positive cap is an operator-authorable hot loop. Clamping to it (the /// obvious Math.Max(0, …)) makes EVERY delay zero, so the pump reconnect-spins as fast From 7eba588305cb5879442a88f9305335475b427893 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:59:11 -0400 Subject: [PATCH 2/2] docs(code-reviews): add Driver.MTConnect + Driver.MTConnect.Contracts findings (first review, b990adae) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driver.MTConnect: 8 findings — 1 High (Driver.MTConnect-003, browse-committed tags are typed String and defeat the probe inference; cause is in AdminUI's RawBrowseCommitMapper, so recorded Open with a recommendation), 3 Medium (001/002 fixed in 6c63bf8f, 004 the missing write.enabled authoring surface, cross-module Open), 4 Low. 5 remain Open, all cross-module or record-only. Driver.MTConnect.Contracts: 1 finding — a Low documentation defect whose false premise concealed Driver.MTConnect-003; fixed in 6c63bf8f. No behavioural issues: the accept-gate-first Inspect contract, the FirstNonBlank alias precedence and the digits-only enum pre-check were all verified as deliberate and correct at HEAD. README.md deliberately not regenerated (out of scope for this review). --- .../Driver.MTConnect.Contracts/findings.md | 76 +++++ code-reviews/Driver.MTConnect/findings.md | 300 ++++++++++++++++++ 2 files changed, 376 insertions(+) create mode 100644 code-reviews/Driver.MTConnect.Contracts/findings.md create mode 100644 code-reviews/Driver.MTConnect/findings.md diff --git a/code-reviews/Driver.MTConnect.Contracts/findings.md b/code-reviews/Driver.MTConnect.Contracts/findings.md new file mode 100644 index 00000000..4b450198 --- /dev/null +++ b/code-reviews/Driver.MTConnect.Contracts/findings.md @@ -0,0 +1,76 @@ +# Code Review — Driver.MTConnect.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts` | +| Reviewer | Claude (per-module review) | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Complete | +| Open findings | 0 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found. `TryFromTagConfig` cannot throw for any input: the only reachable failure of `JsonSerializer.Deserialize` over this all-scalar DTO is `JsonException`, which is caught, and the null/blank input is guarded before it. `SeparatorInsensitiveComparer.Equals` and `GetHashCode` hash and compare exactly the same characters (the classic bucket-mismatch bug is explicitly avoided and commented). `Infer`'s `(uint)sampleCount.Value` cannot overflow — `sampleCount` is an `int` the probe parser already constrained, and the cast is guarded by `> 0`. The `DATA_SET`/`TABLE` demotion is correctly checked **before** the category, so a `DATA_SET` `SAMPLE` cannot escape as `Float64`. | +| 2 | OtOpcUa conventions | No issues found. Zero-dependency leaf over `Core.Abstractions` only, mirroring `Driver.Modbus.Contracts` / `Driver.AbCip.Contracts`; net10.0, nullable enabled, `TreatWarningsAsErrors`. No `JsonStringEnumConverter` on `JsonOptions` — the type-carrying field stays `string?` and is parsed by NAME, with a digits-only pre-check that rejects the numerically-serialized-enum shape before `Enum.TryParse` can accept `"8"` as an ordinal. The absent `GenerateDocumentationFile` matches every other `*.Contracts` project in the fleet (checked against Modbus/AbCip/S7/Mqtt), so it is convention rather than an omission. | +| 3 | Concurrency & thread safety | No issues found. Every type here is immutable or stateless: two `sealed record`s, a `readonly record struct`, a `FrozenSet` built once in a static initialiser, and a stateless singleton comparer. No mutable static state, no I/O, no locks needed. | +| 4 | Error handling & resilience | No issues found. `TryFromTagConfig` is total (never throws, returns a verdict), `Inspect` is total, and `IsAcceptedDataTypeName` treats blank as accepted so an unauthored type falls through to the Agent's own declaration rather than rejecting a tag. A present-but-invalid type rejects rather than silently defaulting — the sibling drivers' strict-enum posture, and the fix for Gitea #549. | +| 5 | Security | No issues found. No I/O, no credentials, no logging, no reflection over untrusted input; the JSON reader is bounded by a fixed DTO shape. `Inspect`'s messages echo the operator's own authored value and the valid enum names, which is the intended operator-facing detail and carries nothing secret. | +| 6 | Performance & resource management | No issues found. `MTConnectInferredType` is a `readonly record struct` so the discovery hot path allocates nothing; `NumericEventTypes` is a `FrozenSet` consulted through a comparer rather than by normalising the input, so the lookup needs no temporary string; `Infer` trims via `ReadOnlySpan`. Nothing here is disposable. | +| 7 | Design-document adherence | 1 finding — `Driver.MTConnect.Contracts-001`. The accept-gate-first contract from the #549–#559 arc is correctly implemented: `Inspect` asks `TryFromTagConfig` for the verdict and only explains a rejection, so it can never be stricter than the runtime; `IsAcceptedDataTypeName` is the single authority both the runtime and `MTConnectTagConfigModel.Validate` consult; and `FirstNonBlank(DriverDataType, DataType)` is deliberate, mirrored by the AdminUI model's load order and its normalise-onto-`dataType`-and-drop-the-alias save (#550). The `reference.TrimStart().FirstOrDefault() != '{'` early return in `Inspect` is likewise fleet convention — all ten driver factories carry the identical guard — so it was reviewed and not raised. | +| 8 | Code organization & conventions | No issues found. The parse lives here as one authority rather than being mirrored into the AdminUI, which is exactly the drift the arc's design argues against; the DTO is private to the factory; the Options pattern is honoured and every collection defaults to empty rather than null. | +| 9 | Testing coverage | No issues found. The project has no test project of its own (fleet convention for `*.Contracts`); it is covered from `Driver.MTConnect.Tests` (`MTConnectDataTypeInferenceTests` is a golden table including a probe-vs-streams spelling-parity case; `MTConnectRawTagBindingTests` and `MTConnectDriverOptionsTests` drive the factory) and from `AdminUI.Tests`, whose TagConfig parity corpus consumes `IsAcceptedDataTypeName` / `Inspect` through `MTConnectTagConfigModel`. Both suites were run after this review's changes: 538 and 1466 pass. | +| 10 | Documentation & comments | 1 finding — `Driver.MTConnect.Contracts-001`. The XML documentation is otherwise excellent and records the *reason* for each rule — why the digits-only pre-check exists, why the alias out-ranks the canonical key, why the comparer is separator-insensitive (the probe document's `PART_COUNT` vs the streams document's `PartCount`), and why blank is accepted. | + +## Findings + + + +#### Driver.MTConnect.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs:31` · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:86` | +| Status | Resolved | + +**Description:** Both public types documented a call graph that no longer exists, and in one case the +false premise concealed a live defect rather than merely misinforming a reader. + +1. `MTConnectDataTypeInference`'s class remarks justify living in `.Contracts` because "three call + sites must agree byte-for-byte": `ITagDiscovery.DiscoverAsync`, "the universal browser's commit + path (writes the browsed leaf into `TagConfig`)", and "the AdminUI typed tag editor (shows the + inferred type and offers an override)". At HEAD a `grep` for `MTConnectDataTypeInference` outside + this project returns only the driver: `RawBrowseCommitMapper` never calls it, and + `MTConnectTagConfigModel` explicitly documents that it does **not** recompute the inference. Two + of the three named callers do not exist. +2. `MTConnectDriverOptions.RawTags` describes the coercion-type precedence and states that the + `/probe`-inference step "is what makes a browse-committed tag work: `RawBrowseCommitMapper` writes + only the address into the blob, so without it every value from the address picker would publish as + text under a numerically-typed OPC UA node." That premise is false at HEAD — the mapper writes a + full `MTConnectTagConfigModel` blob whose `dataType` defaults to `String` — and the sentence + therefore asserts as *prevented* precisely the outcome that now happens on every browse commit. + +The second is what makes this worth recording rather than a tidy-up: a reader auditing the browse +path would have taken that remark as evidence the case was handled. The underlying defect is +recorded as `Driver.MTConnect-003` (Open, fix belongs in `RawBrowseCommitMapper`). + +**Recommendation:** Correct both remarks to describe the callers that exist, and — rather than +deleting the precedence rule, which is still correct for hand-authored, CSV-imported and API-authored +blobs — state explicitly that the step is not reached by a browse-committed tag today, with a pointer +to the finding that tracks the cause. + +**Resolution:** Fixed `6c63bf8f`, 2026-08-10 — `MTConnectDataTypeInference` now names its two real +callers (`DiscoverAsync` and `MTConnectDriver.InferDefinition`) and carries a warning that the two +it used to name do not call it; `MTConnectDriverOptions.RawTags` carries a warning that the +`/probe` step is unreachable for a browse-committed tag at HEAD. Both point at +`Driver.MTConnect-003`. Comment-only: `Driver.MTConnect.Tests` (538) and `AdminUI.Tests` (1466) both +re-run green. diff --git a/code-reviews/Driver.MTConnect/findings.md b/code-reviews/Driver.MTConnect/findings.md new file mode 100644 index 00000000..2cb22dd6 --- /dev/null +++ b/code-reviews/Driver.MTConnect/findings.md @@ -0,0 +1,300 @@ +# Code Review — Driver.MTConnect + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect` | +| Reviewer | Claude (per-module review) | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Complete | +| Open findings | 5 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | 3 findings — `Driver.MTConnect-002`, `Driver.MTConnect-003`, `Driver.MTConnect-006` | +| 2 | OtOpcUa conventions | No issues found. `DriverType` and the factory's `DriverTypeName` are both aliased to `DriverTypeNames.MTConnect` (the `TwinCat`/`Focas` drift fix); every status code is a NAMED `const` matching an `Opc.Ua.StatusCodes` member, which the `StatusCodeParityTests` reflection guard requires; there is deliberately no `JsonStringEnumConverter` on the config reader, so enum-carrying DTO fields stay `string?` and go through `ParseEnum`. Authorization is never decided here — the driver reports `SecurityClassification` as browse metadata only, and says so explicitly. .NET 10 / AnyCPU, Serilog via `ILogger` with a `NullLogger` default, no backend NuGet. | +| 3 | Concurrency & thread safety | 1 finding — `Driver.MTConnect-005`. The lock discipline is otherwise sound and documented per field: `_lifecycle` (non-reentrant, never entered from the pump or the probe loop), `_subscriptionLock` (never held across an await or a callback; lock order `_lifecycle` → it; released before `StopSampleStreamAsync`), `_probeLock` (field read/write only). `_session` / `_binding` / `_index` are immutable snapshots published by `Volatile.Write` or compare-and-swap, so a lock-free reader cannot pair one initialize's client with another's model. Subscriber callbacks are raised outside every lock with the invocation list walked by hand, so one throwing consumer cannot starve the others. | +| 4 | Error handling & resilience | 1 finding — `Driver.MTConnect-001`. Otherwise strong: failure is never dressed as success (a `/probe` that succeeds while the priming `/current` fails is Faulted, not "healthy with no data"); a quiet end of the `/sample` enumeration is reported rather than read as idle; an unreadable config document faults only a driver that had no working session; `Faulted` is reserved for a failed initialize because `DriverHealthReport` maps it to a `/readyz` 503; `/current` and `/sample` own separate degrade flags so neither papers over the other's failure. | +| 5 | Security | 1 finding — `Driver.MTConnect-004` (a capability gate nobody can turn on, not a weakness). No credential is logged: `MTConnectDriverProbe` builds every message from a scheme/host/port/path redaction that excludes `Uri.UserInfo`, and its last-resort catch forwards only the exception TYPE. The Agent's refusal text reaches a log line, never `DriverHealth.LastError`. Write-back is off at two independent gates and CONDITION items are excluded. `XDocument.Parse`/`Load` run under the BCL default `DtdProcessing.Prohibit`, so the XML paths are not XXE-exposed, and the multipart reader caps its buffer at 32 MB against a hostile Agent. Every caller-supplied URL part is percent-encoded at the seam. | +| 6 | Performance & resource management | 1 finding — `Driver.MTConnect-007`. Otherwise careful: one `/current` per read batch whatever the batch size; one shared `/sample` long poll behind every subscription handle; `FrozenDictionary` for the RawPath↔dataItemId tables; a separator-insensitive comparer that avoids allocating on the discovery hot path; a multipart reader that never buffers more than the part in flight. Each `CancellationTokenSource` is disposed only once its loop is provably gone (an abandoned loop still holds the token), and the two `HttpClient`s exist precisely so the unary deadline stays bounded while the long poll is not. | +| 7 | Design-document adherence | 1 finding — `Driver.MTConnect-008` (stale claims, corrected). The implementation otherwise matches `docs/drivers/MTConnect.md` and `docs/plans/2026-08-05-mtconnect-writeback-design.md`: `IWritable` is observation injection through `AllowPut`, doubly gated and CONDITION-excluded; `RediscoverPolicy.Once` plus an `instanceId` watch; the dormant discovered-node injection path is correctly not relied on. | +| 8 | Code organization & conventions | No issues found. Capability seams are implemented on the concrete type and the factory deliberately returns it un-narrowed, which is why `IWritable` was dispatched with no factory edit. The Options pattern is honoured, the config document has exactly one parser (`ParseOptions`, which the factory delegates to rather than mirroring), and the transport sits behind `IMTConnectAgentClient` so the whole driver is exercised with no socket. | +| 9 | Testing coverage | No issues found. 538 tests across 14 classes cover the lifecycle, read, write, subscribe, discovery, probe, both parsers, the observation index and the multipart framing; each way the stream can be knocked off sequence (gap, `OUT_OF_RANGE`, restart, drop, not-supported) has a test, several with an explicit falsification control. Nothing waits on wall-clock time — the pump exposes its task as the teardown barrier and the probe cadence is an injected scheduler. The two gaps this review found are `Driver.MTConnect-001` / `-002`; both now have tests, each verified red first. | +| 10 | Documentation & comments | 1 finding — `Driver.MTConnect-008`. The XML documentation is unusually thorough and states the reasoning (why a lock is not reentrant, why an exception beats an empty result, why a fallback exists) rather than restating the code. Both stale items found were premises another module had since invalidated, not drifted descriptions of this code. | + +## Findings + + + +#### Driver.MTConnect-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Error handling & resilience | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:1932` | +| Status | Resolved | + +**Description:** The `/sample` pump's `StreamVerdict.Reopen` arm reset the reconnect ladder +unconditionally (`attempt = 0; _reconnectAttempts = 0;`) and reopened the stream with no backoff. +That is right for the two conditions the verdict was written for — a sequence gap and an Agent +`OUT_OF_RANGE` rejection — because the `/current` re-baseline genuinely resolves both, so neither +recurs immediately. + +But the `catch (InvalidDataException)` in `ConsumeStreamAsync` that routes `OUT_OF_RANGE` to a +re-baseline also catches two conditions a `/current` cannot resolve: a chunk +`MTConnectStreamsParser` cannot parse (thrown from inside the `await foreach`) and a part larger +than the multipart reader's 32 MB cap. Against an Agent producing either, the pump ran +`/sample` → parse failure → `/current` (succeeds) → `Reopen` with the ladder reset → `/sample` +again, **with no delay at any point** — an unbounded hot loop issuing two HTTP requests per +iteration as fast as the Agent can answer, and one Warning per iteration. That is exactly the +failure mode every other arm of this pump is written to prevent (the `_streamUnsupported` latch, +`BackoffGrowthFloorMs`, the non-positive-cap fallback). + +**Recommendation:** Use the discriminator the transient arm already uses — did *this* connection +deliver a chunk? A re-baseline behind a stream that delivered stays an immediate reopen on a clean +ladder; one on a connection that delivered nothing advances `BackoffFor` like any other failure. + +**Resolution:** Fixed `6c63bf8f`, 2026-08-10 — the `Reopen` arm now sets +`attempt = outcome.DeliveredAny ? 0 : attempt + 1`, so a re-baseline that resolved nothing backs off +while a buffer roll behind a working stream still reopens instantly. Pinned by +`A_rebaseline_that_delivers_nothing_still_advances_the_reconnect_backoff` (red before the change: +`ReconnectAttempts` stayed 0 across six stream generations) plus the falsification control +`A_rebaseline_after_a_delivered_chunk_still_reopens_immediately`, which fails any "back off on every +re-baseline" over-correction. + +#### Driver.MTConnect-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriverProbe.cs:192` | +| Status | Resolved | + +**Description:** `MTConnectDriverProbe` (the AdminUI Test Connect probe) parsed only `agentUri` from +the driver config and requested `{AgentUri}/probe`, while `MTConnectAgentClient` narrows every real +request to `{AgentUri}/{DeviceName}/probe`. A driver scoped to a device therefore had its +reachability tested against a **different URL from the one it will use**. + +The consequence is the failure-that-looks-like-success shape: a mistyped, since-renamed, or +not-yet-deployed `deviceName` shows GREEN on Test Connect and then Faults the driver at +`InitializeAsync` — a cppagent answers an unknown device path with an `MTConnectError` document +under HTTP 200, which the parser surfaces as `InvalidDataException`. The type's own "reachable ≠ +deployable" remark did not cover this: the examples it names (`requestTimeoutMs`, a malformed tag +entry) do not change the endpoint, and which endpoint is reached is the one thing a reachability +probe exists to establish. + +**Recommendation:** Read `deviceName` into the probe's minimal DTO and pass it into the +`MTConnectDriverOptions` handed to `MTConnectAgentClient`, so the probe issues the driver's own +request path. Keep the deliberate independence from `ParseOptions` — the distinction is "which keys +compose the URL", not "which keys are cheap to read". + +**Resolution:** Fixed `6c63bf8f`, 2026-08-10 — the probe DTO gained `DeviceName` and forwards it, +and the type remarks now state the URL-composing rule. Pinned by +`Configured_deviceName_scopes_the_probe_request_the_way_the_driver_does` (red before the change: +requested `/probe`, expected `/VMC-1/probe`) and the control +`An_absent_deviceName_leaves_the_probe_agent_wide`. + +#### Driver.MTConnect-003 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs:256` (cause) · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:3286` (`BuildBinding` precedence) | +| Status | Open | + +**Description:** **Every browse-committed MTConnect tag is coerced to `String` at runtime while its +OPC UA node is typed from the probe inference**, so a browsed `SAMPLE` publishes text into a +`Double` node. + +`RawBrowseCommitMapper.MapLeaf` does two things with one browsed leaf: + +- it sets the raw tag row's OPC UA `DataType` from the leaf's inferred `DriverDataType` + (`MapDataType`: `Float64` → `Double`, `Int64` → `Int64`); and +- it calls `BuildTagConfig`, whose MTConnect branch is + `new MTConnectTagConfigModel { FullName = address }.ToJson()`. + +`MTConnectTagConfigModel.DataType` defaults to `DriverDataType.String` and `ToJson()` +**unconditionally** writes `dataType`, so the committed blob is +`{"fullName":"dev1_pos","dataType":"String"}`. That is a non-blank authored type, so +`MTConnectTagDefinitionFactory.TryFromTagConfig` returns a definition, `BuildBinding` installs it as +"the deploy-time authority", and step 3 of the documented coercion precedence — the Agent's own +`/probe` declaration via `MTConnectDataTypeInference` — is **never reached for a browse-committed +tag**. The driver's own remark that this step "is what makes a browse-committed tag work" rested on +a premise (`RawBrowseCommitMapper` writes only the address) that was true when written and is false +at HEAD. + +This is the same defect class as Gitea #536, which is exactly why the AbCip, TwinCAT and FOCAS +branches of `BuildTagConfig` forward the browsed native type via `NativeType(addressFields, …)`. +MTConnect was given the typed-model branch without that forwarding. + +Severity is High rather than Medium because the address picker is the primary authoring path for +this driver, the mistyping is silent (no warning at authoring and none at the deploy gate — the blob +is perfectly valid), and the resulting node advertises a numeric type it will never carry. + +**Recommendation:** Cross-module — do not fix in this driver. In `RawBrowseCommitMapper`, forward +the browsed leaf's `DriverDataType` into the MTConnect branch as the other three typed-model +branches already do. `MapLeaf` already holds `driverDataType`; either thread it into +`BuildTagConfig` alongside `addressFields`, or supply it through `addressFields` under the +`dataType` key `NativeType` already reads. Add the MTConnect case to whatever test pins the #536 +behaviour for AbCip/TwinCAT/FOCAS, and assert the **pair** — that the tag row's OPC UA type and the +blob's coercion type agree — rather than either alone: asserting only the blob would pass against a +mapper that had stopped setting the row's type. + +**Resolution:** _(open — the fix belongs in `ZB.MOM.WW.OtOpcUa.AdminUI`, outside this review's +modules. The stale premises this defect invalidated inside these two modules are corrected under +`Driver.MTConnect-008` and `Driver.MTConnect.Contracts-001`.)_ + +#### Driver.MTConnect-004 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Design-document adherence | +| Location | `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/MTConnectDriverForm.razor:1` · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:146` | +| Status | Open | + +**Description:** `IWritable` shipped for this driver on 2026-08-05 (`b29623cf`) behind +`Write.Enabled`, and `docs/drivers/MTConnect.md` §"Two independent gates" tells the operator to set +`"Write": { "Enabled": true }` in the driver config. **There is no way to author that through the +product.** `MTConnectDriverForm` is the only editor `DriverFormMaps` maps for this driver type, it +exposes no write section, and its header comment still asserts "MTConnect is read-only — there are +no write knobs to author". `DriverConfigModal` renders the typed form only (no raw-JSON escape +hatch), and the AdminUI exposes no driver-instance API — its only `Map*` endpoints are the deploy +and script-analysis ones. So the documented enablement path exists only as a direct edit of +`DriverInstance.DriverConfig` in the config DB. + +Mitigating, and worth knowing before fixing: `MTConnectDriverForm.BuildConfigJson` preserves unknown +**top-level** keys, so a `write` block introduced by any other means is *not* destroyed by a later +edit through the form. The capability is unreachable rather than fragile. + +**Recommendation:** Cross-module — add a "Write-back" section to `MTConnectDriverForm` with an +`Enabled` switch and form text stating that it is observation injection, that the Agent's own +`AllowPut` is the other gate, and that it does not by itself make any node writable over OPC UA +(`Tag.AccessLevel` still composes the `CurrentWrite` bit). Add `Write` to the form's mirrored +`MTConnectConfigDto` so the value round-trips, and delete the now-false "MTConnect is read-only" +comment. Until then the driver-side gate is correct and fails closed, which is why this is Medium +rather than High. + +**Resolution:** _(open — the fix belongs in `ZB.MOM.WW.OtOpcUa.AdminUI`, outside this review's +modules.)_ + +#### Driver.MTConnect-005 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Concurrency & thread safety | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:2655` · `:689` · `:797` · `:1425` | +| Status | Open | + +**Description:** `_options` is the one piece of shared driver state published by a **plain** field +write (`_options = options;` at the `StartCoreAsync` commit point) and read lock-free from other +threads — `ReadAsync`, `WriteAsync`, `DiscoverAsync`, `HostName`, `DegradeAfterReadFailure`. Every +other value published at that same commit point (`_session`, `_binding`, `_index`, +`_announcedInstanceId`) uses `Volatile.Write` and is read with `Volatile.Read`, and the field +remarks explain at length why that matters. + +The practical exposure is small — a reference-typed field cannot tear, and the `Volatile.Write` of +`_binding` two statements later acts as a release fence for it — but a reader that reads `_options` +*before* it reads `_binding` (which both `ReadAsync` and `WriteAsync` do) has no acquire ordering +between them, so it can in principle judge a batch against a stale options document paired with the +new tag table. The worst observable outcome is one batch judged against the previous `Write.Enabled` +or `RequestTimeoutMs`. + +**Recommendation:** Publish and read `_options` with `Volatile.Write` / `Volatile.Read` like its +three siblings, or fold it into the `AgentSession` / `TagBinding` snapshot so the "read once, as one +consistent unit" rule this file already argues for covers it too. Record-only for now: the change +touches ~10 read sites and the exposure does not justify unreviewed churn. + +**Resolution:** _(open)_ + +#### Driver.MTConnect-006 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:914` | +| Status | Open | + +**Description:** `TryFormatValue` refuses `null` and `Array`, then renders everything else with +`Convert.ToString(value, InvariantCulture)`. Its own remark gives the right reason for the array +guard — `Convert.ToString` falls back to `object.ToString()` for a non-`IConvertible` value, so an +array would be injected into the Agent as the literal text `System.Double[]`, which the Agent stores +happily and which stays invisible until someone reads it back. That reasoning is not specific to +arrays: any other non-`IConvertible` value reaching this method (an `Opc.Ua.ExtensionObject`, an +`XmlElement`, a `List` from a non-SDK caller) is injected as its type name under a Good status. + +Not reachable today by the production path — the SDK coerces a written value against the node's +`DataType` before `DriverInstanceActor.HandleWriteAsync` builds the `WriteRequest`, and every +MTConnect tag is a scalar Boolean/numeric/String — hence Low. + +**Recommendation:** Refuse on the property the remark actually relies on rather than on one type +that happens to have it: `if (value is null or Array or not IConvertible) return false;`. `string`, +`bool`, every numeric and `DateTime` are `IConvertible`, so the accepted set is unchanged. + +**Resolution:** _(open)_ + +#### Driver.MTConnect-007 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectAgentClient.cs:85` | +| Status | Open | + +**Description:** `MTConnectAgentClient.Dispose()` is a plain check-then-set on a non-volatile +`_disposed` field, so two concurrent disposals can both pass the guard. The second then calls +`_disposeCts.Cancel()` on an already-disposed `CancellationTokenSource`, which throws +`ObjectDisposedException`. The class documents that disposal is safe against an in-flight +*enumeration* but claims nothing about concurrent disposal, and in practice the driver disposes only +under `_lifecycle` and wraps the call in `SafeDispose`, so the throw is caught and traced — hence +Low rather than Medium. + +**Recommendation:** Guard with `Interlocked.Exchange(ref _disposed, 1) != 0` over an `int` field, or +state on `Dispose` that it is not thread-safe and is called only under the driver's lifecycle lock. + +**Resolution:** _(open)_ + +#### Driver.MTConnect-008 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:3210` · `docs/drivers/MTConnect.md:169` · `MTConnectDriver.cs:1227`/`:1245`/`:1255` | +| Status | Resolved | + +**Description:** Three stale or self-contradicting pieces of documentation, each of which would lead +a reader to a wrong conclusion: + +1. `BuildBinding`'s remarks state that the probe-inference step of the coercion precedence "is what + makes a browse-committed tag work: `RawBrowseCommitMapper` writes only the address into the + blob". That premise is false at HEAD and the step is in fact dead for browse-committed tags — see + `Driver.MTConnect-003`. +2. `docs/drivers/MTConnect.md` §"FullName == DataItem `id`" says `address` "is the field name + `RawBrowseCommitMapper` writes" for MTConnect. It writes `fullName`; `address` is the legacy + spelling that must stay readable. +3. `DiscoverAsync` reads `var options = _options;` once, with an explicit comment that a concurrent + re-initialize must not scope half the walk to one device and half to another — then logs + `_options.AgentUri` (the field) at all three of its log sites. Harmless in effect, but it + contradicts the rule stated two lines above it, which is how such a rule gets broken later. + +**Recommendation:** Correct (1) and (2) to describe HEAD and cross-reference the live defect rather +than deleting the rule, which is still correct for hand-authored, CSV and API blobs; make (3) use +the snapshot it deliberately took. + +**Resolution:** Fixed `6c63bf8f`, 2026-08-10 — `BuildBinding` and `docs/drivers/MTConnect.md` now +carry an explicit warning naming `Driver.MTConnect-003` and the mapper branch responsible, and the +three `DiscoverAsync` log sites use the once-read `options` snapshot.