merge: Mqtt Contracts+Browser code review — wildcard PUBLISH refusal (whole-driver outage path), Sparkplug segment shape rules, rebirth-scope wildcard gate; 12 findings

2026-08-10 07:00:22 -04:00
parent 978eeda753 9ca6042f57
commit c58e833aee
9 changed files with 760 additions and 7 deletions
@@ -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
+
+<!-- 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.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)_
@@ -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
+
+<!-- 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)_
@@ -720,6 +720,19 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession
        return true;
    }

+    /// <summary>
+    ///     Whether an operator-supplied scope segment may be used to <b>address</b> an NCMD. Everything
+    ///     <see cref="IsUsableLabel"/> requires, plus no MQTT wildcard: a group/edge-node id becomes a
+    ///     topic segment of the published command, and MQTT-3.3.2-2 forbids a wildcard in a PUBLISH
+    ///     topic. Kept separate from <see cref="IsUsableLabel"/> on purpose — that one also judges
+    ///     Sparkplug <i>metric</i> names, which are arbitrary publisher-chosen UTF-8 and are never part
+    ///     of a topic, so tightening it there would drop legitimate metrics from the tree.
+    /// </summary>
+    /// <param name="segment">The candidate scope segment.</param>
+    /// <returns><see langword="true"/> when an NCMD may be addressed with it.</returns>
+    private static bool IsUsableRebirthSegment(string segment) =>
+        IsUsableLabel(segment) && segment.IndexOf('+') < 0 && segment.IndexOf('#') < 0;
+
    /// <summary>Test seam over <see cref="Observe"/> that takes the payload as text.</summary>
    /// <param name="topic">The observed topic.</param>
    /// <param name="payload">The message body as text, or <see langword="null"/> for an empty payload.</param>
@@ -895,7 +908,14 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession
        }

        // 3. A bare {group}/{edgeNode} the window has never seen — the prime rebirth target.
-        if (segments.Length == 2 && IsUsableLabel(segments[0]) && IsUsableLabel(segments[1]))
+        //    ⚠ This is the ONE path that takes an id the session never parsed off the wire, so it is the
+        //    one that must check for a wildcard. IsUsableLabel deliberately permits '+'/'#' (a Sparkplug
+        //    METRIC name is arbitrary UTF-8 and could legitimately contain either), so it is not enough
+        //    here: `Plant1/#` would have assembled `spBv1.0/Plant1/NCMD/#`, a PUBLISH the MQTT spec
+        //    forbids, which a conformant broker answers by closing the browse connection — while `+`
+        //    would fan one operator click across every matching edge node. Cases 1 and 2 above cannot
+        //    reach this shape: their ids come off SparkplugTopic.TryParse, which rejects both characters.
+        if (segments.Length == 2 && IsUsableRebirthSegment(segments[0]) && IsUsableRebirthSegment(segments[1]))
        {
            return [(segments[0], segments[1])];
        }
@@ -0,0 +1,63 @@
+namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt;
+
+/// <summary>
+///     The two shape rules an authored MQTT address must satisfy to be <em>publishable</em>, shared by
+///     the deploy-time inspector (<see cref="MqttTagDefinitionFactory.Inspect"/>) and the write encoder
+///     (<see cref="MqttWriteEncoder"/>) so the warning an operator is shown and the refusal the driver
+///     makes can never disagree.
+/// </summary>
+/// <remarks>
+///     <para>
+///         <b>Neither rule gates the READ path, and that asymmetry is the point.</b> A tag whose
+///         subscription <c>topic</c> carries a wildcard still reads (ambiguously — hence
+///         <see cref="MqttTagDefinitionFactory.Inspect"/>'s long-standing advisory), and a Sparkplug tag
+///         with a malformed id still parses. What these rules decide is whether an <em>outbound</em>
+///         message could legally be addressed at all, so they belong to the write encoder and to the
+///         inspector, never to the mappers' accept gate — which must stay exactly as strict as the
+///         runtime reader (Gitea #551).
+///     </para>
+///     <para>
+///         <b>Why a wildcard write destination is a refusal rather than a best effort.</b> MQTT-3.3.2-2
+///         forbids a wildcard in a PUBLISH topic, and a conformant broker answers a protocol violation by
+///         closing the network connection — which for this driver is the one connection every
+///         subscription lives on. Publishing anyway would turn one mis-authored tag into a whole-driver
+///         outage; refusing costs that tag its write and nothing else.
+///     </para>
+/// </remarks>
+internal static class MqttAddressRules
+{
+    /// <summary>The MQTT wildcard characters — illegal in any concrete (published or subscribed) topic.</summary>
+    private static readonly char[] TopicWildcards = ['+', '#'];
+
+    /// <summary>
+    ///     Characters that cannot appear in a <b>single MQTT topic segment</b>, which is what a Sparkplug
+    ///     group / edge-node / device id is. <c>/</c> joins the wildcards here because it would silently
+    ///     re-shape the command topic into a different address rather than making it unpublishable — and
+    ///     because a decoded incoming id can never contain one, so an authored id that does is a binding
+    ///     that could never have resolved.
+    /// </summary>
+    private static readonly char[] SegmentIllegalChars = ['/', '+', '#'];
+
+    /// <summary>
+    ///     Whether <paramref name="topic"/> carries an MQTT wildcard (<c>+</c> / <c>#</c>).
+    /// </summary>
+    /// <param name="topic">A topic string; a <see langword="null"/>/empty one carries none.</param>
+    /// <returns><see langword="true"/> when a wildcard is present.</returns>
+    internal static bool HasWildcard(string? topic) =>
+        !string.IsNullOrEmpty(topic) && topic.IndexOfAny(TopicWildcards) >= 0;
+
+    /// <summary>
+    ///     Whether <paramref name="id"/> is unusable as a single MQTT topic segment — i.e. as a Sparkplug
+    ///     group, edge-node or device id.
+    /// </summary>
+    /// <param name="id">The authored id; a <see langword="null"/>/empty one is not judged here (absence
+    /// is handled by the mappers' own presence checks, and <c>deviceId</c> is legitimately absent).</param>
+    /// <returns><see langword="true"/> when the id contains <c>/</c>, <c>+</c> or <c>#</c>.</returns>
+    /// <remarks>
+    ///     Deliberately NOT applied to <c>metricName</c>: a metric name rides in the Sparkplug
+    ///     <em>payload</em>, not in the topic, and Sparkplug's own canonical names use <c>/</c> (e.g.
+    ///     <c>Node Control/Rebirth</c>). Applying it there would refuse every conformant node-control tag.
+    /// </remarks>
+    internal static bool IsIllegalTopicSegment(string? id) =>
+        !string.IsNullOrEmpty(id) && id.IndexOfAny(SegmentIllegalChars) >= 0;
+}
@@ -47,6 +47,14 @@ public static class MqttTagDefinitionFactory
    /// <summary>The MQTT wildcard characters — illegal in a <em>tag's</em> (concrete) subscription topic.</summary>
    private static readonly char[] TopicWildcards = ['+', '#'];

+    /// <summary>
+    ///     The Sparkplug address keys that are single MQTT topic segments, and so cannot carry <c>/</c>,
+    ///     <c>+</c> or <c>#</c>. <see cref="MqttTagConfigKeys.MetricName"/> is deliberately absent — it
+    ///     rides in the payload, not the topic. See <see cref="MqttAddressRules.IsIllegalTopicSegment"/>.
+    /// </summary>
+    private static readonly string[] SparkplugSegmentKeys =
+        [MqttTagConfigKeys.GroupId, MqttTagConfigKeys.EdgeNodeId, MqttTagConfigKeys.DeviceId];
+
    /// <summary>
    ///     Maps an authored <c>TagConfig</c> object to a typed definition keyed by <paramref name="rawPath"/>.
    ///     The input is always an authored TagConfig JSON object (there is no name-vs-blob heuristic in v3),
@@ -265,9 +273,13 @@ public static class MqttTagDefinitionFactory
    ///         </item>
    ///         <item>
    ///             <b>Advisories</b> — a <b>wildcard</b> tag topic (<c>+</c> / <c>#</c>), which the mapper
-    ///             deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; and a
-    ///             present-but-invalid <c>payloadFormat</c> on an ACCEPTED blob, which can only mean the
-    ///             Sparkplug reader took it and the Plain reader would not (Gitea #557 item 2).
+    ///             deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; a wildcard
+    ///             <c>writeTopic</c>, which reads fine but can never be published to, so every write is
+    ///             refused <c>BadNotWritable</c>; a Sparkplug <c>groupId</c>/<c>edgeNodeId</c>/
+    ///             <c>deviceId</c> carrying a character no MQTT topic segment can hold, which makes the
+    ///             tag permanently unbindable; and a present-but-invalid <c>payloadFormat</c> on an
+    ///             ACCEPTED blob, which can only mean the Sparkplug reader took it and the Plain reader
+    ///             would not (Gitea #557 item 2).
    ///         </item>
    ///     </list>
    ///     Empty for a clean, readable blob, for a blank one, or for one that is not an equipment-tag
@@ -370,6 +382,36 @@ public static class MqttTagDefinitionFactory
                "concrete, or the tag will be fed by every matching topic.");
        }

+        // The write destination gets the same treatment as the read topic, and for a sharper reason: a
+        // wildcard cannot be PUBLISHED to at all (MQTT-3.3.2-2), so the tag reads perfectly while every
+        // write is refused with BadNotWritable — a split nothing else surfaces. The AdminUI's typed
+        // editor already refuses this, but it is only ONE authoring path (the raw-JSON textarea, CSV
+        // import and the API all reach the deploy gate directly), so leaving it out here made the
+        // authoring surface stricter than the deploy gate — the drift this inspector exists to close.
+        var writeTopic = ReadString(root, MqttTagConfigKeys.WriteTopic);
+        if (MqttAddressRules.HasWildcard(writeTopic))
+        {
+            advisories.Add(
+                $"value '{writeTopic}' for 'writeTopic' contains an MQTT wildcard (+ or #); a wildcard "
+                + "cannot be published to, so the tag reads normally but every write is refused "
+                + "(BadNotWritable).");
+        }
+
+        // Same shape, Sparkplug side: a group/edge-node/device id is a single MQTT topic segment, so a
+        // decoded incoming id can never contain '/', '+' or '#'. An authored one that does is a binding
+        // that can never resolve — the mapper accepts it (it checks presence, not shape), so this is the
+        // only place it can be said. `metricName` is deliberately exempt: it rides in the payload, and
+        // Sparkplug's own canonical names use '/'.
+        foreach (var key in SparkplugSegmentKeys)
+        {
+            var value = ReadString(root, key);
+            if (!MqttAddressRules.IsIllegalTopicSegment(value)) continue;
+            advisories.Add(
+                $"value '{value}' for '{key}' contains a character ('/', '+' or '#') that cannot appear "
+                + "in an MQTT topic segment; a decoded Sparkplug id never contains one, so this tag can "
+                + "never bind and will read BadNodeIdUnknown in Sparkplug B mode.");
+        }
+
        // Gitea #557 item 2 — the ONE key whose verdict depends on the DRIVER'S mode, reported as an
        // advisory because no tag-level gate can see that mode. A present-but-invalid payloadFormat is
        // fatal to the Plain parse and invisible to the Sparkplug one, so a union accept here can only
@@ -19,9 +19,11 @@ public enum MqttWriteEncodeError
    None = 0,

    /// <summary>
-    ///     The tag declares no write destination: a Plain tag with no <c>writeTopic</c>. Deliberately a
-    ///     hard refusal — see <see cref="MqttWriteEncoder"/>'s remarks on why a silent no-op would be
-    ///     worse than the honest read-only behaviour this replaced.
+    ///     The tag declares no <b>usable</b> write destination: a Plain tag with no <c>writeTopic</c> or
+    ///     one carrying an MQTT wildcard, or a Sparkplug tag whose binding tuple is incomplete or whose
+    ///     group/edge-node/device id is not a legal topic segment (see <see cref="MqttAddressRules"/>).
+    ///     Deliberately a hard refusal — see <see cref="MqttWriteEncoder"/>'s remarks on why a silent
+    ///     no-op would be worse than the honest read-only behaviour this replaced.
    /// </summary>
    NoWriteDestination,

@@ -121,6 +123,17 @@ public static class MqttWriteEncoder
            return MqttWriteEncodeError.NoWriteDestination;
        }

+        // A wildcard topic is not a destination — MQTT-3.3.2-2 forbids one in a PUBLISH, and a
+        // conformant broker answers a protocol violation by CLOSING the connection, which is the same
+        // connection every one of this driver's subscriptions lives on. So publishing anyway would turn
+        // one mis-authored tag into a whole-driver outage. `NoWriteDestination` (→ BadNotWritable) is the
+        // right verdict rather than a new error: the tag genuinely has nowhere a write could be sent, and
+        // the read path is untouched. See MqttAddressRules.
+        if (MqttAddressRules.HasWildcard(def.WriteTopic))
+        {
+            return MqttWriteEncodeError.NoWriteDestination;
+        }
+
        if (value is null)
        {
            // No MQTT payload means "the value is null" — a zero-length payload is a legitimate
@@ -175,6 +188,18 @@ public static class MqttWriteEncoder
            return MqttWriteEncodeError.NoWriteDestination;
        }

+        // …and an id that is not a legal topic SEGMENT is the same failure by a different route: the
+        // tuple is assembled into the NCMD/DCMD topic, so a '/' would silently address a different node
+        // than the tag names while '+'/'#' would make the PUBLISH a protocol violation (see the Plain
+        // branch). `metricName` is exempt on purpose — it rides in the payload, and Sparkplug's own
+        // canonical names use '/'.
+        if (MqttAddressRules.IsIllegalTopicSegment(groupId)
+            || MqttAddressRules.IsIllegalTopicSegment(edgeNodeId)
+            || MqttAddressRules.IsIllegalTopicSegment(def.DeviceId))
+        {
+            return MqttWriteEncodeError.NoWriteDestination;
+        }
+
        if (value is null)
        {
            return MqttWriteEncodeError.ValueNotRepresentable;
@@ -854,6 +854,27 @@ public sealed class MqttBrowseSessionTests
        s.PublishCountForTest.ShouldBe(0);
    }

+    [Theory]
+    [InlineData("Plant1/#")]
+    [InlineData("Plant1/+")]
+    [InlineData("+/EdgeA")]
+    [InlineData("#/EdgeA")]
+    public async Task RequestRebirthAsync_WildcardScope_PublishesNothing(string scope)
+    {
+        // SparkplugRebirthScope (the AdminUI's typed input) rejects a wildcard and its own remarks say
+        // it is "advisory, not the gate" because ResolveRebirthTargets "refuses an unusable one BEFORE
+        // any message leaves". That claim has to be true here: an unobserved `{group}/{edgeNode}` scope
+        // is honoured on purpose (a node that has not birthed is the prime rebirth target), and the
+        // label check behind it looked only at length and control characters. A wildcard segment would
+        // therefore have published `spBv1.0/Plant1/NCMD/#` — a PUBLISH the MQTT spec forbids, which a
+        // conformant broker answers by closing the browse connection.
+        await using var s = new MqttBrowseSession(MqttMode.SparkplugB);
+
+        await Should.ThrowAsync<ArgumentException>(
+            async () => await s.RequestRebirthAsync(scope, TestContext.Current.CancellationToken));
+        s.PublishCountForTest.ShouldBe(0);
+    }
+
    [Fact]
    public async Task RequestRebirthAsync_InPlainMode_IsRefused()
    {
@@ -252,6 +252,54 @@ public sealed class MqttInspectAcceptGateTests
        warnings.ShouldNotContain(w => w.Contains("Plain mode"), "an unserved tag is not a mode-dependent case");
    }

+    // ---- The two authoring-only rules the deploy gate used to pass over in silence ----
+
+    /// <summary>
+    ///     <c>MqttTagConfigModel.ValidatePlain</c> refuses a wildcard <c>writeTopic</c> outright, but the
+    ///     typed editor is only ONE authoring path — the raw-JSON textarea, CSV import and the API all
+    ///     reach the deploy gate directly. A wildcard cannot be published to at all (MQTT-3.3.2-2), so the
+    ///     tag reads perfectly while every write is refused; that has to be said somewhere the operator
+    ///     sees it, and an ADVISORY is the right class because the blob IS accepted.
+    /// </summary>
+    [Fact]
+    public void A_wildcard_write_topic_is_advised_on_an_accepted_blob()
+    {
+        const string Blob = """{"topic":"plant/line1/temp","writeTopic":"plant/+/set"}""";
+
+        EitherModeAccepts(Blob).ShouldBeTrue("the tag still READS fine — only the write is unaddressable");
+        MqttTagDefinitionFactory.Inspect(Blob)
+            .ShouldContain(w => w.Contains("writeTopic") && w.Contains("wildcard"));
+    }
+
+    /// <summary>
+    ///     A Sparkplug group / edge-node / device id is a single MQTT topic segment, so a decoded incoming
+    ///     id can never contain <c>/</c>, <c>+</c> or <c>#</c> — an authored one that does is a binding
+    ///     that can never resolve, and the mapper accepts it (it validates presence, not shape). The
+    ///     AdminUI model calls this out in words and refuses the save; the deploy gate said nothing.
+    /// </summary>
+    /// <param name="key">The key the advisory must name.</param>
+    /// <param name="blob">The tag config carrying an unbindable id.</param>
+    [Theory]
+    [InlineData("groupId", """{"groupId":"Ot/Sim","edgeNodeId":"EdgeA","metricName":"T"}""")]
+    [InlineData("edgeNodeId", """{"groupId":"OtSim","edgeNodeId":"Edge+A","metricName":"T"}""")]
+    [InlineData("deviceId", """{"groupId":"OtSim","edgeNodeId":"EdgeA","deviceId":"F#1","metricName":"T"}""")]
+    public void A_sparkplug_id_that_is_not_a_topic_segment_is_advised(string key, string blob)
+    {
+        EitherModeAccepts(blob).ShouldBeTrue("the reader accepts it — which is exactly why it must be advised");
+        MqttTagDefinitionFactory.Inspect(blob).ShouldContain(w => w.Contains(key));
+    }
+
+    /// <summary>
+    ///     The falsification control for the rule above: <c>metricName</c> is NOT a topic segment — it
+    ///     rides in the payload — and Sparkplug's own canonical names use <c>/</c>. Advising it would
+    ///     report every conformant <c>Node Control/Rebirth</c> tag as broken.
+    /// </summary>
+    [Fact]
+    public void A_metric_name_containing_a_slash_is_never_advised()
+        => MqttTagDefinitionFactory
+            .Inspect("""{"groupId":"OtSim","edgeNodeId":"EdgeA","metricName":"Node Control/Rebirth"}""")
+            .ShouldBeEmpty();
+
    /// <summary>
    ///     Both gates must keep agreeing on the VERDICT (Gitea #557 item 2). The #554 authoring consult in
    ///     <c>MqttTagConfigModel</c> asks the identical union and only words a REFUSAL, so an advisory can
@@ -237,6 +237,58 @@ public sealed class MqttWriteEncoderTests
        EncodeTopic(MqttMode.SparkplugB, both).ShouldBe("spBv1.0/OtOpcUaSim/DCMD/EdgeA/Filler1");
    }

+    // ---- A destination that cannot be published to is not a destination -------------------------
+
+    [Theory]
+    [InlineData("factory/+/set")]
+    [InlineData("factory/oven/#")]
+    public void A_wildcard_write_topic_is_refused_rather_than_published(string writeTopic)
+    {
+        // MQTT-3.3.2-2: a PUBLISH topic MUST NOT contain a wildcard, and a conformant broker answers a
+        // protocol violation by CLOSING the connection — which, for this driver, is the same connection
+        // every subscription lives on. So one write to a mis-authored tag takes the WHOLE driver dark,
+        // not just that tag. The typed AdminUI editor already refuses this, but a raw-JSON / CSV /
+        // API-authored blob reaches the encoder untouched, so the refusal has to live here too.
+        MqttWriteEncoder.TryEncode(MqttMode.Plain, Plain(writeTopic), 1, out var encoded)
+            .ShouldBe(MqttWriteEncodeError.NoWriteDestination);
+        encoded.Topic.ShouldBeNull();
+    }
+
+    [Theory]
+    [InlineData("Ot/Sim", "EdgeA", null)]
+    [InlineData("OtSim", "Edge+A", null)]
+    [InlineData("OtSim", "EdgeA", "Fill#er")]
+    [InlineData("OtSim", "EdgeA", "Fill/er")]
+    public void A_binding_tuple_that_is_not_a_legal_topic_segment_has_no_command_address(
+        string groupId, string edgeNodeId, string? deviceId)
+    {
+        // The same protocol hazard on the Sparkplug side, plus one the Plain side does not have: the
+        // tuple IS the command topic, so a '/' silently re-shapes it into a DIFFERENT address (an
+        // NCMD for group "Ot" node "Sim" rather than the tag's own node) while '+'/'#' make it
+        // unpublishable. Refusing is the only honest verdict for all three.
+        var def = Sparkplug(deviceId) with { GroupId = groupId, EdgeNodeId = edgeNodeId };
+
+        MqttWriteEncoder.TryEncode(MqttMode.SparkplugB, def, 1.0, out _)
+            .ShouldBe(MqttWriteEncodeError.NoWriteDestination);
+    }
+
+    [Fact]
+    public void A_metric_name_containing_a_slash_still_encodes()
+    {
+        // The falsification control for the rule above. Sparkplug's own canonical metric names use '/'
+        // (`Node Control/Rebirth`), and metricName rides in the PAYLOAD, never in the topic — so the
+        // segment rule must not reach it or every conformant node-control write would be refused.
+        MqttWriteEncoder.TryEncode(
+                MqttMode.SparkplugB,
+                Sparkplug(deviceId: null, DriverDataType.Boolean, metricName: "Node Control/Rebirth"),
+                true,
+                out var encoded)
+            .ShouldBe(MqttWriteEncodeError.None);
+
+        encoded.Topic.ShouldBe("spBv1.0/OtOpcUaSim/NCMD/EdgeA");
+        Payload.Parser.ParseFrom(encoded.Payload).Metrics[0].Name.ShouldBe("Node Control/Rebirth");
+    }
+
    private static string EncodeTopic(MqttMode mode, MqttTagDefinition def)
    {
        MqttWriteEncoder.TryEncode(mode, def, 1.0, out var encoded).ShouldBe(MqttWriteEncodeError.None);