From 9ca6042f5771b6b20ce5bf5cec0f6ccb54d74589 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:58:08 -0400 Subject: [PATCH] docs(code-reviews): first review of Driver.Mqtt.Contracts + Driver.Mqtt.Browser at b990adae MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 10 checklist categories completed for both modules. 12 findings total: 1 High + 2 Medium fixed under TDD (6eace4bb, 31182137), 1 Medium fixed in the browser, 8 Low recorded Open with concrete recommendations. README.md deliberately NOT regenerated — regen-readme.py is left for the caller. --- code-reviews/Driver.Mqtt.Browser/findings.md | 200 +++++++++++++ .../Driver.Mqtt.Contracts/findings.md | 282 ++++++++++++++++++ 2 files changed, 482 insertions(+) create mode 100644 code-reviews/Driver.Mqtt.Browser/findings.md create mode 100644 code-reviews/Driver.Mqtt.Contracts/findings.md diff --git a/code-reviews/Driver.Mqtt.Browser/findings.md b/code-reviews/Driver.Mqtt.Browser/findings.md new file mode 100644 index 00000000..9308f7b3 --- /dev/null +++ b/code-reviews/Driver.Mqtt.Browser/findings.md @@ -0,0 +1,200 @@ +# Code Review — Driver.Mqtt.Browser + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser` | +| Reviewer | Claude | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 4 | + +## Scope + +The whole module: `MqttDriverBrowser.cs` (192 LOC), `MqttBrowseSession.cs` (1,213 LOC) +and the csproj. No generated code in this project. + +## 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 | 2 findings — `Driver.Mqtt.Browser-001`, `-004` | +| 2 | OtOpcUa conventions | No issues found. The browser parses the driver's `DriverConfig` through the shared `MqttJson.Options` rather than a private `JsonSerializerOptions`, which is what keeps the picker from accepting a `mode`/`protocolVersion` spelling the deployed driver rejects (the repo's documented systemic enum bug). `DriverType` is taken from `DriverTypeNames.Mqtt`, not a literal. No authorization decision is made here — `BrowserSessionService` enforces `DriverOperator`, and the session explicitly holds no user identity, matching the "authorization at the server layer, never in driver-level code" rule. | +| 3 | Concurrency & thread safety | 2 findings — `Driver.Mqtt.Browser-003` and the ordering half of `-004`; both benign. The design is otherwise sound: the tree is built from `ConcurrentDictionary` with `StringComparer.Ordinal`, every mutation is lock-free, per-node observation records are swapped as whole references under `Volatile.Read`/`Write` so a UI thread never sees a half-written value, `_disposed` is an `Interlocked.Exchange` one-shot, and `Observe` is written to never throw on MQTTnet's dispatcher thread. | +| 4 | Error handling & resilience | No issues found. `OpenAsync` bounds connect + subscribe with a clamped budget (5–30 s) and disposes the session — which owns the client — on any failure. `DisposeAsync` is idempotent, safe on a never-connected client, bounds the courtesy DISCONNECT at 5 s and swallows-and-logs both teardown steps. `ExpandAsync` returns empty for an unknown node id rather than throwing, correctly treating a stale id as an ordinary race against a tree that grows while it is browsed. | +| 5 | Security | 1 finding — `Driver.Mqtt.Browser-001`. The module's central safety property is otherwise well built and genuinely checkable: every outbound message funnels through one private counted `PublishAsync`, reached only via a private nested `RebirthTransport` (so holding the session does not confer a publish-anything capability), and `PublishCountForTest` makes "browsing publishes nothing" assertable rather than aspirational. The Last-Will landmine is documented at length and guarded by a reflection test. Logs carry host/port/mode/filter only — never credentials or payload bodies. Metric names off the wire are **rejected** rather than scrubbed, precisely because a scrubbed name would be committed as a different binding key. | +| 6 | Performance & resource management | 1 finding — `Driver.Mqtt.Browser-005`. Bounds are otherwise thorough and correctly reasoned: `PayloadInspectMaxBytes` caps decode work per message (the node cap alone would not, since a large payload on a known topic creates no node), `MaxBirthPayloadBytes` bounds the protobuf parse, `Utf8Strict` is cached rather than allocated per message, and `Scrub` stack-allocates under 128 chars. | +| 7 | Design-document adherence | No issues found. Matches `docs/plans/2026-05-28-driver-browsers-design.md` and the MQTT design's §3.1/§4: births-only Sparkplug discovery (a DATA metric carries an alias and no name), node-level metrics hung directly under their edge node rather than under a synthesised "(node)" folder, no payload sniffing in Sparkplug mode, and `AddressFields` keyed by `MqttTagConfigKeys` so the commit mapper reads the address off fields rather than re-parsing a node id. The Gitea #512 layering fix is real and measured — the csproj comment even corrects its own earlier over-claim about `Core.dll`. | +| 8 | Code organization & conventions | No issues found. Two files, one responsibility each (open/connect vs. accumulate/serve). The `MetricSeparator` (`::`) decision — so a node-level metric `Plant1/EdgeA/Temperature` cannot be confused with a device folder of the same shape — is correct and, importantly, nothing re-parses it: the rebirth target and the address tuple are both **read** off the scope stamped on the node, mirroring the v3 `AddressSpaceRealm` discipline. | +| 9 | Testing coverage | No issues found. `MqttBrowseSessionTests` + `MqttBrowserLayeringTests` (in `Driver.Mqtt.Tests`, which reaches internals via `InternalsVisibleTo`) cover both tree shapes, the caps, the markers, rebirth scoping and the publish-count guarantee. The Sparkplug test seams feed a **real** encoded birth through the real `Observe` path — topic parse, protobuf decode and tree build all exercised rather than mocked around — which is why `-001` was reachable as a test at all. | +| 10 | Documentation & comments | 1 finding — the false claim documented in `Driver.Mqtt.Browser-001` was in a *sibling* file (`SparkplugRebirthScope`) asserting a guarantee this module did not provide; it is now true. Otherwise the XML docs are unusually good, and several carry the reasoning that makes the code reviewable at all (why a browse client id must differ from the driver's; why a device folder states its kind rather than inferring it from child count; why a DataSet cell is always `{column}/{row}`). | + +## Findings + + + +### Driver.Mqtt.Browser-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:898`, `:712` | +| Status | Resolved | + +**Description:** `ResolveRebirthTargets`' third branch — a bare `{group}/{edgeNode}` the +window has never observed, honoured on purpose because a node that has not birthed is the +prime rebirth target — gated its two segments on `IsUsableLabel`, which checks length and +control characters only. MQTT wildcards passed. A scope of `Plant1/#` therefore assembled +and published `spBv1.0/Plant1/NCMD/#`: a PUBLISH the MQTT spec forbids (MQTT-3.3.2-2), +which a conformant broker answers by closing the connection, killing the operator's browse +session mid-pick. `Plant1/+` is the worse shape — a broker that tolerates it would fan one +operator click across every matching edge node, the exact broker-storm `MaxGroupRebirthNodes` +exists to prevent, and with no cap applied because the group-enumeration branch was never +entered. + +What makes this more than theoretical is the documented division of responsibility. +`SparkplugRebirthScope` (the AdminUI's typed input) rejects wildcards and states in its own +remarks that it "is advisory, not the gate" because *"`MqttBrowseSession.ResolveRebirthTargets` +re-resolves the scope and refuses an unusable one **before any message leaves**"* — a +guarantee this module did not actually provide. Anything reaching `BrowserSessionService` +without going through that Razor component (the API surface) had no wildcard check at all. + +Branches 1 and 2 were never affected: their ids come off `SparkplugTopic.TryParse`, which +already rejects `+`/`#` per segment. + +**Recommendation:** Reject wildcard segments on the raw-input branch. Do **not** tighten +`IsUsableLabel` itself — it also judges Sparkplug *metric* names, which are arbitrary +publisher-chosen UTF-8, are never part of a topic, and legitimately contain characters a +topic segment cannot; tightening it there would silently drop legitimate metrics from the +picker tree. + +**Resolution:** Fixed in `31182137` (2026-08-10) — a separate `IsUsableRebirthSegment` +guards branch 3 (`IsUsableLabel` plus no `+`/`#`), leaving metric-name handling untouched; +pinned by `MqttBrowseSessionTests.RequestRebirthAsync_WildcardScope_PublishesNothing` over +four wildcard shapes, each asserting `PublishCountForTest == 0`. + +### Driver.Mqtt.Browser-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:469` | +| Status | Open | + +**Description:** The plain-mode and Sparkplug-mode paths disagree about what may become a +tree label. `ObserveSparkplugBirth` runs every group/edge-node/device/metric identifier +through `IsUsableLabel`, whose doc explains the reason: "a metric name is arbitrary UTF-8 +chosen by the publisher, and an embedded newline or ANSI escape would break the picker's +single-line rows." `ObservePlainTopic` applies only the length half of that (`MaxTopicChars`, +`MaxSegmentChars`) and no control-character check, so a plain topic segment goes into +`TopicNode.Segment` — the picker's displayed label — unfiltered. MQTT forbids only `U+0000` +in a topic name, so a publisher can legally put `\n` or an ANSI escape in one. + +Related and even smaller: a topic with an empty segment (`a//b`, or a trailing `/`) creates a +node whose label is the empty string, rendering as a blank picker row. The committed address +is still correct — `AddressFields[topic]` carries `node.Path`, the true topic — so this is a +display defect, not a mis-binding. + +**Recommendation:** Extend the existing pre-pass in `ObservePlainTopic` (which already walks +every segment to check length, specifically so a rejected topic leaves no half-built path +behind) to reject a segment containing a control character, setting the same `_oversized` +marker. The plain-mode marker text would need widening from "Some topics were too long to +show" to cover malformed names — the Sparkplug branch already words its marker that way +("oversized or malformed names"), so the two can converge. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Browser-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Concurrency & thread safety | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:685` | +| Status | Open | + +**Description:** `GetOrCreate` reads `_nodeCount` and compares it to `_nodeCap`, then +increments after a successful `GetOrAdd`. The read and the increment are not atomic with +respect to each other, so N threads can each observe `_nodeCount == _nodeCap - 1` and all +proceed, overshooting the cap by up to N-1 nodes. MQTTnet delivers on a single dispatcher +per client, so N is 1 in practice today and the overshoot is unobservable; it becomes real +only if a future change fans observation across threads. + +There is a second, already-documented race in the same method: a node is inserted into its +parent's `Children` before it is published to `_byPath`, so an `ExpandAsync` landing in that +window returns empty for a node the tree already contains. The class remarks accept this +explicitly ("a stale id is an ordinary race, not an error") and it self-heals on the next +call, so it is noted rather than filed. + +**Recommendation:** If the concurrency assumption ever changes, replace the read/increment +pair with a single `Interlocked.Increment` compared against the cap, decrementing on the +lost-race path (`!ReferenceEquals(child, created)`). Until then, a comment stating the +single-dispatcher assumption the current shape relies on would be enough. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Browser-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:1079` | +| Status | Open | + +**Description:** `Project` orders a level by `n.Segment`, but the dictionary is keyed by +`n.Path` — and the whole reason for that keying (documented on `GetOrCreate`) is that in +Sparkplug mode two siblings *can* share a `Segment`: a device folder `Plant1/EdgeA/Temp` and +an edge-node-level metric `Plant1/EdgeA::Temp` are different nodes with the same label. +`OrderBy` is stable with respect to the input sequence, but the input is +`ConcurrentDictionary.Values`, whose enumeration order is not guaranteed and can change as +the tree grows. So two such siblings can swap places between successive `ExpandAsync` calls +— the picker's list reorders under the operator mid-browse, and the two rows are visually +identical, so it is not obvious which one is being clicked. + +The comment on `Project` claims "ordered so the picker is stable", which is only true when +labels are unique. + +**Recommendation:** Make the sort total by adding `.ThenBy(n => n.Path, StringComparer.Ordinal)`, +and/or order folders before leaves so a device and a same-named metric are visually +separated. Either is a one-line change; a test observing a device and an edge-node metric of +the same name would pin it. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Browser-005 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:59`, `:81` | +| Status | Open | + +**Description:** The per-session memory ceiling is higher than the constants suggest at a +glance. `DefaultNodeCap` is 50,000 nodes, and each `TopicNode` retains its full `Path` +(up to `MaxTopicChars` = 1,024 chars), its `Segment` (up to 256) and a snippet (up to 64), +plus its own `ConcurrentDictionary`. Every node is also held twice — once in its parent's +`Children`, once in the flat `_byPath` index. A worst case near those bounds is on the order +of 100+ MB for a single browse window, and a plain-mode open with no `TopicPrefix` +subscribes `#`, i.e. the entire broker, which is exactly the shape that would approach it. +Several concurrent pickers multiply it. + +This is bounded and self-limiting, not a leak — `DisposeAsync` clears both maps and the +session reaper (outside this module) bounds lifetime — and the `MaxTopicChars` remarks show +the authors reasoned about precisely this ("50 000 nodes near that ceiling is a multi-GB +tree"). What is missing is any bound on the *aggregate* across sessions. + +**Recommendation:** No code change needed in this module. Worth confirming that +`BrowserSessionService` caps concurrent sessions per user and that its idle reaper interval +is short enough to matter; if it does not, cap there rather than lowering `DefaultNodeCap`, +which would make the picker less useful on a genuinely large broker. + +**Resolution:** _(open)_ diff --git a/code-reviews/Driver.Mqtt.Contracts/findings.md b/code-reviews/Driver.Mqtt.Contracts/findings.md new file mode 100644 index 00000000..348eb820 --- /dev/null +++ b/code-reviews/Driver.Mqtt.Contracts/findings.md @@ -0,0 +1,282 @@ +# Code Review — Driver.Mqtt.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts` | +| Reviewer | Claude | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 4 | + +## Scope + +Handwritten sources only (~2,760 LOC across 16 `.cs` files): the top-level contract +types (`MqttDriverOptions`, `MqttTagDefinition`, `MqttTagDefinitionFactory`, +`MqttTagConfigKeys`, `MqttWriteEncoder`, `MqttJson`, `MqttLastWill`, +`SparkplugDataType`, and the three enums) plus the handwritten `Sparkplug/` protocol +layer (`SparkplugTopic`, `SparkplugCodec`, `SparkplugFlattener`, +`SparkplugStatePayload`, `RebirthRequester`). + +Generated code skipped: + +- `obj/Debug/net10.0/Protos/SparkplugB.cs` — 6,873 LOC emitted by `Grpc.Tools` + (`protoc`) from `Protos/sparkplug_b.proto` at build time (`GrpcServices="None"`, + message-only). Build output, not checked in, never hand-edited. +- `Protos/sparkplug_b.proto` — the **vendored** Eclipse Tahu schema. Read only to + check the claims made about it elsewhere (proto2 explicit presence; + `optional uint32 datatype = 4`); it is a third-party contract, not this repo's code. + +Note the `Sparkplug/` subdirectory is **not** generated — those five files are +handwritten and were reviewed in full. + +## 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.Mqtt.Contracts-001`, `-002`, `-006` | +| 2 | OtOpcUa conventions | No issues found. `MqttJson.Options` is the single shared `JsonSerializerOptions` carrying `JsonStringEnumConverter`, anchored in `.Contracts` (the only assembly all four seams reference) — the documented systemic driver-enum-serialization bug is closed by construction. `SparkplugDataType` as a `global using` alias for the generated Tahu enum, rather than a hand-duplicated 35-member copy, is the right call and is drift-guarded by a reflection test. The assembly stays transport-free (no MQTTnet reference). | +| 3 | Concurrency & thread safety | No issues found. Every type is `static` + pure or an immutable `record`; the only shared mutable state is `MqttJson.Options`, which `System.Text.Json` freezes on first use. `SparkplugCodec` / `SparkplugFlattener` hold no state and are written (and documented) to be safe on MQTTnet's shared dispatcher thread. | +| 4 | Error handling & resilience | 1 finding — `Driver.Mqtt.Contracts-007`. Otherwise strong: both mappers and both codecs are total functions over arbitrary input. `SparkplugCodec.TryDecode`'s deliberately broad `catch (Exception)` spans the *projection* as well as the parse, which is correct at a dispatcher-thread boundary walking an attacker-shaped object graph. Zero-length-is-invalid and the bounded `MaxDepth`/`MaxMembers` flatten guards are both right. | +| 5 | Security | 1 finding — `Driver.Mqtt.Contracts-001` (a wildcard PUBLISH is a protocol violation a broker answers by dropping the connection: a driver-wide outage triggered by one authored field). Credential handling is otherwise correct — `MqttDriverOptions.PrintMembers` is hand-written so `Password` renders as `***` (and distinguishably for null/empty), and prints `RawTags` as a count so a log of the DTO is not a config dump. No secret is logged anywhere in the assembly. | +| 6 | Performance & resource management | No issues found. `SparkplugCodec` shares one empty member list rather than allocating per scalar metric; `BytesValue` is copied out of the `ByteString` because the projection outlives the parsed message; the flattener's `columns × rows` bound is computed through a `(long)` cast so it cannot overflow. Nothing here owns an `IDisposable`. `Inspect`'s `TrimStart().FirstOrDefault()` allocates, but runs once per tag at deploy time only. | +| 7 | Design-document adherence | 1 finding — `Driver.Mqtt.Contracts-003`. Otherwise matches `docs/plans/2026-07-15-mqtt-sparkplug-driver-design.md` §3.1/§3.5/§3.6/§5.1–5.3 and the #549–#559 arc (strict enum + qos reads, accept-gate-first `Inspect`, structurally separated rejections/advisories, `JsonDocument.Parse` + root-`ValueKind` as the single framing authority). Those were reviewed **as shipped at HEAD** and are correct; nothing here reverts them. | +| 8 | Code organization & conventions | No issues found. Clean layering: `.Contracts` holds the wire schema, the pure codecs and the pure mappers, and is referenced by the runtime driver, the transport, the browser and the AdminUI without any of them reaching back. `MqttTagConfigKeys` single-sources the address key names between producer (browse-commit mapper) and consumer (factory) — the drift it prevents is otherwise a silent `BadNodeIdUnknown`. | +| 9 | Testing coverage | 1 finding — `Driver.Mqtt.Contracts-005` (the unenforced options annotations are untested because there is nothing to test). The module has no test project of its own by design; behaviour is pinned by `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests` (708 at base → 724 after this review) and the AdminUI TagConfig parity corpus (1,466). Coverage of the mappers, `Inspect`, the codec, the flattener, the topic parser and the golden Sparkplug vectors is genuinely thorough, including falsification controls. | +| 10 | Documentation & comments | 1 finding — `Driver.Mqtt.Contracts-004` (a stated strictness principle the code does not apply to one key). XML docs are otherwise exceptionally accurate and carry the *reasons* for non-obvious decisions (why no `ToTagConfig` inverse; why the alias rather than a duplicate enum; why `is_null` wins over the value `oneof`; why a Template definition is refused; why the two STATE bodies share one timestamp). Every claim spot-checked against the code held. | + +## Findings + + + +### Driver.Mqtt.Contracts-001 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs:119` | +| Status | Resolved | + +**Description:** `TryEncodePlain` checked only that `WriteTopic` was non-blank, then +published to it verbatim. MQTT-3.3.2-2 forbids the wildcard characters `+`/`#` in a +PUBLISH topic, and §4.8 requires a server encountering a protocol violation to close the +network connection — which, for this driver, is the single connection every subscription +lives on. So one write to a tag whose `writeTopic` carries a wildcard did not fail that +tag; it dropped the whole driver's ingest session and took every tag on the device to Bad +until the reconnect supervisor recovered. + +The AdminUI's typed editor (`MqttTagConfigModel.ValidatePlain`) already refuses such a +blob, and its own comment says a wildcard "would fail at the broker rather than here" — +but the typed editor is one authoring path of several. The raw-JSON textarea, CSV import +and the deployment API all persist a blob that never passes through it, and `Inspect` said +nothing (see `-003`), so the value reached the encoder unchallenged. + +**Recommendation:** Refuse it in the encoder, mapping to the existing +`MqttWriteEncodeError.NoWriteDestination` (→ `BadNotWritable`) rather than inventing a new +error code — the tag genuinely has nowhere a write could be sent, and +`MqttDriver.MapEncodeError` already handles that member. Leave the read path untouched: +the tag still subscribes and reads normally. + +**Resolution:** Fixed in `6eace4bb` (2026-08-10) — `TryEncodePlain` refuses a wildcard +`writeTopic` as `NoWriteDestination`, with the rule single-sourced in the new internal +`MqttAddressRules.HasWildcard`; pinned by +`MqttWriteEncoderTests.A_wildcard_write_topic_is_refused_rather_than_published`. + +### Driver.Mqtt.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs:169`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs:202` | +| Status | Resolved | + +**Description:** The same hole on the Sparkplug side, with one failure mode the Plain side +does not have. `TryEncodeSparkplug` checked the binding tuple for *presence* only and +handed it to `SparkplugTopic.Format`, which validates its arguments for null/empty but — +unlike `TryParse`, which rejects a segment containing `+`/`#` — applies no character rule +at all. So an authored `groupId` of `Ot/Sim` assembled `spBv1.0/Ot/Sim/NCMD/EdgeA`, a +**well-formed topic addressing a different edge node** than the tag names, and the command +went silently to the wrong device. A `+`/`#` in any of the three ids produced an +unpublishable topic with the connection-drop consequence of `-001`. + +A decoded incoming Sparkplug id can never contain any of those three characters (the topic +parser splits on `/` and rejects wildcards), so an authored id that does is a binding that +could never have resolved anyway — the AdminUI model says exactly this and refuses the +save, but again only on the typed-editor path. + +**Recommendation:** Validate the three id fields as single MQTT topic segments in the +encoder and refuse as `NoWriteDestination`. `metricName` must be **exempt** — it rides in +the payload, not the topic, and Sparkplug's own canonical names use `/` (e.g. +`Node Control/Rebirth`), so a blanket rule would refuse every conformant node-control +write. + +**Resolution:** Fixed in `6eace4bb` (2026-08-10) — `TryEncodeSparkplug` rejects a +`groupId`/`edgeNodeId`/`deviceId` containing `/`, `+` or `#` via +`MqttAddressRules.IsIllegalTopicSegment` while exempting `metricName`; pinned by +`MqttWriteEncoderTests.A_binding_tuple_that_is_not_a_legal_topic_segment_has_no_command_address` +with `A_metric_name_containing_a_slash_still_encodes` as the falsification control. + +### Driver.Mqtt.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Design-document adherence | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs:361` | +| Status | Resolved | + +**Description:** `Inspect` — the deploy-time gate, consumed fleet-wide through +`EquipmentTagConfigInspector` — reported a wildcard **subscription** `topic` but was silent +about both rules `-001` and `-002` describe, even though the AdminUI's typed editor treats +both as hard, save-blocking errors. That is the authoring-stricter-than-the-gate direction +of the drift class the #549–#559 arc exists to close: a blob authored through the raw-JSON +textarea, a CSV import or the API deploys clean and green, and the operator learns about it +only when a write returns `BadNotWritable` (Plain) or the tag never leaves +`BadNodeIdUnknown` (Sparkplug). The Sparkplug case is the worse of the two: a permanently +dead tag with nothing anywhere saying why. + +**Recommendation:** Add both as **advisories**, never rejections — the mappers accept these +blobs and `Inspect`'s accept gate must never be stricter than the runtime (#551). Adding +advisories is safe for the authoring side because `TagConfigParseGate.Describe` consults +`Inspect` only *after* the parse has already refused, and never treats a non-empty +inspection as an error. + +**Resolution:** Fixed in `6eace4bb` (2026-08-10) — `DescribeAdvisories` gained a +wildcard-`writeTopic` advisory and a per-key Sparkplug-segment advisory (the key list +deliberately excludes `metricName`); pinned by +`MqttInspectAcceptGateTests.A_wildcard_write_topic_is_advised_on_an_accepted_blob`, +`A_sparkplug_id_that_is_not_a_topic_segment_is_advised`, and the control +`A_metric_name_containing_a_slash_is_never_advised`. + +### Driver.Mqtt.Contracts-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs:124`, `:234`, `:476` | +| Status | Open | + +**Description:** `retainSeed` is the one behavioural key still read leniently. +`ReadBoolOrDefault` returns the default for any non-boolean JSON value, so +`"retainSeed": "false"` (a string, which a CSV import or a hand-edited blob produces +naturally) silently becomes `true` — the *opposite* of what was authored, and the tag then +seeds its initial value from the broker's retained message when the author asked it not to. +`payloadFormat`/`dataType` (via `TryReadEnumStrict`) and `qos` (via `TryReadQosStrict`) were +deliberately made strict for exactly this reason, and the `qos` comment states the +principle: silently absorbing a malformed value hands the operator a weaker guarantee than +the one they authored, with nothing to show for it. `retainSeed` is not held to it, and no +comment acknowledges the exception. + +Recorded rather than fixed because the two surfaces are currently *consistent* — the +AdminUI model reads `retainSeed` with the same leniency — so nothing is lying to the +operator today, and tightening it changes an accept verdict, which this review should not +do unilaterally. + +**Recommendation:** Either (a) read it strictly, mirroring `TryReadQosStrict`'s +absent/valid/present-but-invalid split, and mirror the same change in `MqttTagConfigModel` +so both gates stay in step; or (b) add an `Inspect` advisory naming the key and the silent +default; or (c) at minimum document the deliberate exception on `ReadBoolOrDefault`. +Option (b) is lowest-risk — it changes no verdict. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Contracts-005 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs:25`, `:71`, `:75`, `:79`, `:83`, `:99`, `:229` | +| Status | Open | + +**Description:** `MqttDriverOptions` / `MqttPlainOptions` carry seven +`System.ComponentModel.DataAnnotations` `[Range]` attributes and **nothing ever runs them**. +Grepping `Validator.TryValidateObject` / `ValidationContext` across all four MQTT projects +returns only the `using` directive in this file. The options are bound by +`JsonSerializer.Deserialize` through `MqttJson.Options`, not by +`IOptions` + `ValidateDataAnnotations`, so the attributes are documentation that reads like +enforcement. + +The consequence that actually escapes is `MqttPlainOptions.DefaultQos` (`[Range(0, 2)]`): +it flows unclamped through `MqttSubscriptionManager` into `BuildFilters`, which propagates +it to the SUBSCRIBE as-is, so a config carrying `"defaultQos": 5` puts an illegal QoS on +the wire. Reachability is limited — the AdminUI's `MqttDriverFormModel` both validates +("Default QoS must be 0, 1 or 2.") and `Math.Clamp`s it, and `MqttDriverProbe` range-checks +`Port` — so only a hand-edited or API-supplied driver config reaches it. `[Range]` on +driver options is also a repo-wide convention (`ModbusDriverOptions`, `S7DriverOptions` do +the same), so this is not an MQTT-specific defect. + +**Recommendation:** Do not fix in this module alone. Either run the annotations once at the +composition seam every driver's config passes through (the factory extension) so the +convention becomes enforcement fleet-wide, or drop the attributes in favour of the explicit +checks the probe and the AdminUI form already carry. If neither, note on each attribute +that it is descriptive. The enforcing consumer (`MqttSubscriptionManager.BuildFilters`) +lives in `Driver.Mqtt`, outside this module. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Contracts-006 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs:281` | +| Status | Open | + +**Description:** Two edge cases in the `DriverDataType.DateTime` write arm. First, +`(ulong)…ToUnixTimeMilliseconds()` on a pre-1970 value wraps a negative `long` into a very +large `ulong`, so writing `1969-12-31` publishes a timestamp roughly 584 million years in +the future instead of being refused — the "unrepresentable values are refused, never +coerced" rule the surrounding code states is not applied to the epoch bound. Second, +`Convert.ToDateTime(value, InvariantCulture).ToUniversalTime()` treats a +`DateTimeKind.Unspecified` input as *local* time, so the same authored instant encodes +differently depending on the node's timezone — which matters on a redundant pair whose two +nodes need not agree. + +Impact is low: Sparkplug `DateTime` command metrics are rare, and the OPC UA write path +supplies `DateTimeKind.Utc` values in practice. + +**Recommendation:** Return `ValueNotRepresentable` when `ToUnixTimeMilliseconds()` is +negative, and convert with an explicit `DateTime.SpecifyKind(…, DateTimeKind.Utc)` for an +`Unspecified` input (or refuse it, which is more in keeping with the file's own posture). +Add a test for each. + +**Resolution:** _(open)_ + +### Driver.Mqtt.Contracts-007 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Error handling & resilience | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs:202`, `:232` | +| Status | Open | + +**Description:** `SparkplugTopic` validates asymmetrically. `TryParse` enforces +`IsValidSegment` (non-empty, no `+`/`#`) on every segment it reads, but `Format` and +`FormatState` enforce only `ArgumentException.ThrowIfNullOrEmpty` on +`groupId`/`edgeNodeId`/`hostId` and apply **no** check at all to `deviceId`. The type is +therefore not round-trip-closed: `SparkplugTopic.Parse(x.ToTopicString())` can throw +`FormatException` for an instance built by hand (a `deviceId` of `""` emits a trailing `/`; +one containing `/` silently adds a segment). Every *production* caller is now guarded — +`MqttWriteEncoder` since `-002`, `RebirthRequester` via the browser fix +(`Driver.Mqtt.Browser-001`) — so this is a latent trap for the next caller rather than a +live defect. + +**Recommendation:** Apply `IsValidSegment` inside `Format`/`FormatState` (throwing +`ArgumentException`, matching the existing contract) so the invariant lives on the type +that owns it rather than in each caller. Note this makes `Format` throw where it previously +succeeded, so the encoder's own guards must stay — `TryEncode` returns an error rather than +throwing, and that contract must not change. A round-trip property test +(`Parse(t.ToTopicString()) == t` over the parser's own accepted corpus) would pin it. + +**Resolution:** _(open)_