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

<!-- 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 | 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.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 (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.