lmxopcua/code-reviews/Driver.Mqtt.Contracts/findings.md

# 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

<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
     never reused. Findings are never deleted — close them by changing Status and
     completing Resolution. -->

### 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)_