fix(inspect): trim before the framing check; stop advisories swallowing the backstop (#556); pin the MQTT payloadFormat mode contract (#557 item 2)

#556 item 1 — leading whitespace defeated every Inspect's pre-filter. Seven factories (Modbus/S7/AbCip/AbLegacy/TwinCAT/FOCAS/Mqtt) tested `reference[0] != '{'`, so a padded blob returned EMPTY and a tag the driver refuses deployed clean. The check now trims first, matching the idiom Sql/Calculation/MTConnect/OpcUaClient already used. The raw reference is still what is handed to the mapper — the driver reads the stored blob, not a trimmed copy — so the gate keeps mirroring the runtime rather than becoming lenient. Both directions are pinned: a padded blob the mapper accepts still draws nothing, and one that is not object-framed AFTER trimming still draws nothing (it is not an equipment-tag TagConfig at all). Six of the seven mappers hand the blob to a whitespace-tolerant JsonDocument.Parse, so their verdict is unchanged by padding. MQTT is the exception: both its readers carry the same framing gate, so a padded blob is genuinely refused — which is why the untrimmed pre-filter was a real defect there and not a cosmetic mismatch. #556 item 2 — FOCAS and MQTT appended advisories unconditionally and then gated the catch-all rejection message on `warnings.Count == 0`, so an advisory (which fires on ACCEPTED blobs too, and says nothing about a refusal) silently swallowed the only message naming a refusal nothing else can. The two classes now accumulate in SEPARATE lists and the backstop gates on the rejection count alone — structural, not string matching. MQTT's swallow was reachable in production (a padded blob its readers refuse while every describe branch finds the parsed document well-formed); FOCAS's backstop is currently unreachable by audit, so its guard is protective. #557 item 2 — decided and pinned: KEEP the union verdict. Evidence: FromSparkplugTagConfig hard-codes PayloadFormat=Json and never consults the key; only MqttSubscriptionManager (the Plain ingest path) decodes by it; and MqttTagConfigModel.ToJson deliberately omits the key entirely in Sparkplug mode (#513). So a Sparkplug driver genuinely serves the tag and rejecting it would be stricter than the runtime. The cost — served behaviour depends on the driver's MqttMode, which no tag row records — is paid by an ADVISORY naming the key and the mode dependency, not by changing the verdict, so both gates keep agreeing. The same invalid value on a blob the union REFUSES stays a rejection; the class is decided by the accept gate, never by the key. Falsified: reverting Modbus's trim reddens the padded-rejection theory; neutering MQTT's backstop guard to the pre-#556 shape reddens the advisory-plus-unnameable-rejection test; disabling the #557 advisory reddens the contract pin; and for FOCAS, removing a describe branch alone stays green (the backstop covers it) while additionally neutering the guard goes red.
2026-08-10 03:07:18 -04:00
parent ca3968cc31
commit f5475868f7
14 changed files with 752 additions and 56 deletions
@@ -122,16 +122,37 @@ public static class FocasTagDefinitionFactory
    ///     </list>
    ///     Empty for a clean, accepted blob, for a blank one, or for one that is not an equipment-tag
    ///     object. Never throws.
+    ///     <para>
+    ///         The distinction is <b>structural</b>, not a matter of wording (Gitea #556 item 2): the two
+    ///         classes accumulate in separate lists, and the catch-all backstop — the only thing that
+    ///         reports a refusal neither <see cref="DescribeRejection"/> branch can name — fires on "no
+    ///         REJECTION was produced", never on "no warning at all". Gated on the total, an advisory
+    ///         (which fires on an accepted blob too) would silently swallow it.
+    ///     </para>
    /// </summary>
    /// <param name="reference">The equipment tag's TagConfig JSON.</param>
    /// <returns>The warnings; empty when clean.</returns>
    public static IReadOnlyList<string> Inspect(string reference)
    {
-        var warnings = new List<string>();
-        if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings;
+        // Gitea #556 item 2: the two classes live in SEPARATE lists, and the backstop below gates on the
+        // REJECTION list alone. Sharing one list meant an advisory (which fires on an accepted blob too)
+        // could satisfy a `warnings.Count == 0` guard and silently swallow the catch-all rejection message
+        // — the only thing that reports a refusal neither DescribeRejection branch can name. The split is
+        // structural: nothing string-matches a message to work out which class it is.
+        var rejections = new List<string>();
+        var advisories = new List<string>();
+
+        // Gitea #556 item 1: the framing check TRIMS FIRST. FromTagConfig hands the blob straight to
+        // JsonDocument.Parse, which tolerates leading whitespace — so an untrimmed check let " {…}" be
+        // read (and possibly REFUSED) by the driver while drawing zero deploy warnings. A reference that
+        // still does not start with '{' after trimming is not an equipment-tag TagConfig at all and keeps
+        // drawing nothing.
+        if (string.IsNullOrWhiteSpace(reference) || reference.TrimStart().FirstOrDefault() != '{')
+            return rejections;

        // The accept gate: only a blob the driver REFUSES gets a rejection diagnosis. The advisories below
-        // apply either way — see the summary.
+        // apply either way — see the summary. The RAW reference is handed to the mapper: the driver reads
+        // the stored blob, not a trimmed copy of it.
        var accepted = FromTagConfig(reference, InspectionRawPath, out _);

        try
@@ -140,27 +161,43 @@ public static class FocasTagDefinitionFactory
            var root = doc.RootElement;
            if (root.ValueKind != JsonValueKind.Object)
            {
-                warnings.Add("FOCAS TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown).");
-                return warnings;
+                rejections.Add("FOCAS TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown).");
+                return rejections;
            }

-            if (!accepted) DescribeRejection(root, warnings);
-            DescribeAdvisories(root, warnings);
+            if (!accepted) DescribeRejection(root, rejections);
+            DescribeAdvisories(root, advisories);
        }
        catch (JsonException)
        {
-            warnings.Add("FOCAS TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown).");
-            return warnings;
+            rejections.Add("FOCAS TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown).");
+            return rejections;
        }

-        if (!accepted && warnings.Count == 0)
+        // Backstop: the mapper refused for a reason no branch above names. Gated on the REJECTION count,
+        // never on the total — an advisory says nothing about why the tag was refused.
+        if (!accepted && rejections.Count == 0)
        {
-            warnings.Add(
+            rejections.Add(
                "FOCAS TagConfig was rejected by the driver's tag reader — the driver will skip this tag "
                + "and it will read BadNodeIdUnknown.");
        }

-        return warnings;
+        return Combine(rejections, advisories);
+    }
+
+    /// <summary>
+    ///     Flattens the two warning classes into the single list <see cref="Inspect"/> returns, rejections
+    ///     first — the order the pre-split implementation produced, so no caller sees a reordering.
+    /// </summary>
+    /// <param name="rejections">The rejection-class warnings.</param>
+    /// <param name="advisories">The advisory-class warnings.</param>
+    /// <returns>The combined list.</returns>
+    private static List<string> Combine(List<string> rejections, List<string> advisories)
+    {
+        if (advisories.Count == 0) return rejections;
+        rejections.AddRange(advisories);
+        return rejections;
    }

    /// <summary>
@@ -168,21 +205,22 @@ public static class FocasTagDefinitionFactory
    ///     same order. Runs only after the mapper has already refused, so it can never invent strictness.
    /// </summary>
    /// <param name="root">The TagConfig root object.</param>
-    /// <param name="warnings">The list to append warnings to.</param>
-    private static void DescribeRejection(JsonElement root, List<string> warnings)
+    /// <param name="rejections">The REJECTION-class list to append to (never the advisory list — see
+    /// <see cref="Inspect"/>'s backstop).</param>
+    private static void DescribeRejection(JsonElement root, List<string> rejections)
    {
        if (!root.TryGetProperty("address", out var addr) || addr.ValueKind != JsonValueKind.String)
        {
-            warnings.Add(
+            rejections.Add(
                "FOCAS TagConfig has no string 'address' — the driver will skip this tag and it will read "
                + "BadNodeIdUnknown.");
        }
        else if (string.IsNullOrWhiteSpace(addr.GetString()))
        {
-            warnings.Add("FOCAS TagConfig has a blank 'address' — the tag will not resolve (BadNodeIdUnknown).");
+            rejections.Add("FOCAS TagConfig has a blank 'address' — the tag will not resolve (BadNodeIdUnknown).");
        }

-        if (TagConfigJson.DescribeInvalidEnum<FocasDataType>(root, "dataType") is { } w) warnings.Add(w);
+        if (TagConfigJson.DescribeInvalidEnum<FocasDataType>(root, "dataType") is { } w) rejections.Add(w);
    }

    /// <summary>
@@ -190,8 +228,9 @@ public static class FocasTagDefinitionFactory
    ///     "advisories" bullet on <see cref="Inspect"/>.
    /// </summary>
    /// <param name="root">The TagConfig root object.</param>
-    /// <param name="warnings">The list to append warnings to.</param>
-    private static void DescribeAdvisories(JsonElement root, List<string> warnings)
+    /// <param name="advisories">The ADVISORY-class list to append to. These fire whether or not the mapper
+    /// accepted, so they must never reach the rejection list — see <see cref="Inspect"/>'s backstop.</param>
+    private static void DescribeAdvisories(JsonElement root, List<string> advisories)
    {
        // Gitea #540: an address the driver cannot parse is skipped at init and reads BadNodeIdUnknown
        // forever, while driver health stays Healthy with zero errors. The AdminUI modal rejects it
@@ -202,13 +241,13 @@ public static class FocasTagDefinitionFactory
            && !string.IsNullOrWhiteSpace(address)
            && FocasAddress.TryParse(address) is null)
        {
-            warnings.Add(
+            advisories.Add(
                $"FOCAS address '{address.Trim()}' is not parseable (expected X0.0 / R100, " +
                "PARAM:1020, MACRO:500 or FIXED:Identity/SeriesNumber) — the driver will skip " +
                "this tag at startup and it will read BadNodeIdUnknown.");
        }

        if (root.TryGetProperty("writable", out var wr) && wr.ValueKind == JsonValueKind.True)
-            warnings.Add("FOCAS wire is read-only — an authored 'writable:true' still fails at the wire with BadNotWritable.");
+            advisories.Add("FOCAS wire is read-only — an authored 'writable:true' still fails at the wire with BadNotWritable.");
    }
 }