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	0

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	Resolved

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 (2026-08-10): Fixed in 93f70504 — option (a), the strict read, chosen over the "lowest-risk" advisory. The #549–#559 arc's settled contract is that the parse is the accept authority and advisories ride blobs the parse has already ACCEPTED; an advisory here would have left the driver still serving the inverse of what was authored, and the review's own reason for recording rather than fixing ("the two surfaces are currently consistent") is satisfied without a hand-mirrored rule — see below. Every sibling behavioural key in this factory (payloadFormat, dataType, qos) is already strict, and retainSeed is the sharper case of the two: a defaulted qos is a weaker delivery guarantee, whereas a defaulted retainSeed is the inverse of the authored intent whenever the author wrote false — the only value worth writing at all.

Both mappers now read it through a new TryReadBoolStrict, mirroring TryReadQosStrict's absent / valid / present-but-invalid split, and DescribeRejection gained a matching DescribeInvalidBool branch (a rejection, never an advisory: unlike payloadFormat, both mappers read the key, so it can never land on an accepted blob).

No second gate was bolted on outside Parse, and MqttTagConfigModel needed no mirrored rule. Its Validate() already ends in TagConfigParseGate.Describe, which asks this parse about the blob the model was loaded from and words the refusal from this Inspect — so tightening the factory tightened the authoring surface in the same change, with no drift surface created. The model's RetainSeed doc now records that its own load stays lenient (a non-boolean reads as "unset") and why that is safe.

Pinned by MqttTagDefinitionFactoryTests.FromTagConfig_MalformedRetainSeed_RejectsStrict / FromSparkplugTagConfig_MalformedRetainSeed_RejectsStrict (over "false", "true", 0, 1, "yes", null), Inspect_MalformedRetainSeed_Warns, and — as the falsification control that a reader simply refusing every blob carrying the key would fail — FromTagConfig_WellFormedRetainSeed_IsStillHonoured. 11 RED before the fix. The authoring↔deploy parity is pinned separately, on the corpus that would catch a future divergence: TagConfigNonEnumParityTests.Mqtt_refuses_a_retainSeed_its_reader_refuses and its paired Mqtt_accepts_a_retainSeed_its_reader_accepts.

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	Resolved

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.Clamps 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 (2026-08-10): Fixed in 93f70504 — the attributes were deleted, not wired up, and the one consequence that actually escaped was closed at the enforcing consumer.

Wiring them would have broken deliberate behaviour, which settles the "either/or" the recommendation leaves open. Two of the seven contradicted the real authority: [Range(1, …)] on KeepAliveSeconds forbids the zero MqttDriver.Normalize explicitly admits ("zero keep-alive is legal MQTT; only a negative one is nonsense"), and the same attribute on MaxPayloadBytes forbids the non-positive value this type's own remarks document as "falls back to the driver's own 1 MiB default". Running the annotations would have refused two configurations the driver is written to accept — the "two disagreeing authorities" outcome, arrived at by enforcement rather than by neglect. So the bounds stay where they are applied (MqttDriver.Normalize, MqttSubscriptionManager's constructor, MqttConnection.NextBackoff), each property names its own, and the type remarks record why there are no DataAnnotations here and why adding one would be documentation that reads like enforcement.

MqttPlainOptions.DefaultQos — the escape the entry identifies — is now clamped in MqttSubscriptionManager's constructor, the same single enforcement point that already bounds MaxPayloadBytes. Clamp rather than fall back to the default, deliberately: it matches what MqttDriverFormModel.Normalize already persists (Math.Clamp(DefaultQos, 0, 2)), so the authoring surface and the driver cannot reach different answers for one authored number. Pinned by MqttSubscriptionManagerTests.DefaultQos_OutsideTheLegalRange_NeverReachesTheSubscribe, which asserts the QoS on the fake transport's actual SUBSCRIBE rather than on a property — 5 ⇒ 2 and -1 ⇒ 0 were RED, with 0 ⇒ 0 / 2 ⇒ 2 green throughout as the controls.

Scope note: this closes the MQTT half only. The entry's observation that [Range] on driver options is a repo-wide convention (ModbusDriverOptions, S7DriverOptions) still stands, and those attributes are still unenforced — this change does not touch them, and the evidence above argues the fleet-wide answer is deletion plus real bounds at each use site rather than a composition-seam Validator.TryValidateObject.

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	Resolved

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 (2026-08-10): Both fixed in 93f70504.

The epoch bound: ToUnixTimeMilliseconds() is now taken into a long, and a negative one returns ValueNotRepresentable before anything is written to the metric — Sparkplug's DateTime is an unsigned ms-since-epoch, so a pre-1970 instant has no representation at all and the surrounding "refused, never coerced" rule applies unchanged.

The kind: an Unspecified input is converted with DateTime.SpecifyKind(…, DateTimeKind.Utc) rather than ToUniversalTime(). The entry offers refusal as "more in keeping with the file's own posture", and it was not taken — Convert.ToDateTime("2026-08-10T12:00:00"), an ordinary authored string, produces exactly this kind, so refusing would reject a perfectly reasonable write to fix a determinism problem that reading it as UTC also fixes.

Pinned by MqttWriteEncoderTests.A_pre_epoch_date_time_is_refused_rather_than_wrapped (three instants, incl. DateTime.MinValue) with A_post_epoch_date_time_still_encodes_to_its_unix_milliseconds as the falsification control — a guard that refused every DateTime would satisfy the theory alone — and An_unspecified_kind_date_time_encodes_as_utc_not_as_node_local_time, which asserts the Unspecified and the same-instant-UTC encodings are byte-identical. ⚠️ That last one can only go red on a host whose local timezone is not UTC (it was RED here under both EDT and TZ=Asia/Tokyo, and is green under TZ=UTC either way) — which is the defect itself, and the reason it is written as an equality between two encodings rather than against a hard-coded number.

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	Resolved

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 (2026-08-10): Fixed in 93f70504 — Format and FormatState now apply the parser's own IsValidSegment to groupId / edgeNodeId / deviceId / hostId through a shared ThrowIfNotASegment, throwing ArgumentException as the existing contract does.

One deliberate addition beyond the recommendation's literal wording: the format-side check also rejects /. IsValidSegment has no reason to test for one — TryParse has already split on it — but on this side it is the difference between one id and two, and it is precisely the shape the entry describes as "silently adds a segment". Without it the round-trip property the recommendation asks for would not actually be total, since such a topic re-parses to nothing.

The caution the entry raises is respected: MqttWriteEncoder's own guards stay, so TryEncode still returns NoWriteDestination for these ids rather than propagating a throw, and that contract is unchanged. The one production caller that can now throw where it previously produced a malformed topic is SparkplugPrimaryHost.StateTopic (a wildcard hostId); it is reached only from MqttDriver.StartPrimaryHostAsync, which is wrapped in a catch that logs a warning and leaves ingest untouched by design — so the effect is "the host claim declines with a reason" instead of "publish a protocol violation and lose the connection".

Pinned by SparkplugTopicTests.Format_SegmentThatIsNotALegalTopicSegment_Throws (9 cases: +/# in each id, / in group and device, and an empty deviceId) and FormatState_HostIdThatIsNotALegalTopicSegment_Throws — 9 RED before the fix — with Format_UnusualButLegalSegments_AreStillAccepted as the falsification control (a Sparkplug id is otherwise opaque: spaces, dots, dashes and accents are all legitimate, and a guard that refused everything would satisfy the throw theories). The closure property is Format_Output_AlwaysReParsesToTheSameInstance, asserted over the formatter's own output across every non-STATE message type plus FormatState, rather than over a hand-written topic list, so a future formatting change cannot leave the corpus describing a shape the type no longer emits.