Merge pull request #55 from agessaman/fix/mqtt-preset-file-parity

Share one preset table between channels; enforce it byte-for-byte

.github/workflows/check-mqtt-preset-parity.yml

@@ -1,7 +1,11 @@
 name: Check MQTT Preset Name Parity
 
-# Ensures observer-firmware and observer-firmware-dev share the same built-in
-# MQTT preset *names* (config details may differ). See scripts/check_mqtt_preset_parity.py.
+# Ensures observer-firmware and observer-firmware-dev ship an identical
+# src/helpers/MQTTPresets.h. A slot's preset is stored in /mqtt.json by name, and
+# firmware that does not know a name repairs it to "none" and writes the file
+# back, so a node that rolls back between channels loses that slot for good.
+# Behaviour that differs per channel belongs in MQTTPresetPolicy.h, which is not
+# compared. See scripts/check_mqtt_preset_parity.py.
 
 permissions:
   contents: read
@@ -81,9 +85,9 @@ jobs:
       - name: Self-test checker
         run: python3 scripts/check_mqtt_preset_parity.py --self-test
 
-      - name: Compare preset names
+      - name: Compare preset tables
         run: |
-          python3 scripts/check_mqtt_preset_parity.py \
+          python3 scripts/check_mqtt_preset_parity.py --exact \
             /tmp/preset-parity/prod.h \
             /tmp/preset-parity/dev.h \
             --label-a observer-firmware \

scripts/check_mqtt_preset_parity.py

@@ -1,13 +1,25 @@
 #!/usr/bin/env python3
-"""Compare MQTT built-in preset *names* between two MQTTPresets.h files.
+"""Compare the MQTT broker preset table between two MQTTPresets.h files.
 
-Only the first string field of each ``MQTT_PRESETS`` entry is compared (set
-equality, case-sensitive). URL, auth, CA, keepalive, and credentials are
-ignored so channel branches may diverge on config details without failing CI.
+With ``--exact`` (what CI uses) the two files must be byte-identical. The table
+is fleet state, not just a build detail: a slot's preset is stored in
+/mqtt.json by NAME, and firmware that does not know a name does not merely
+ignore it — MQTTPrefsSerializer repairs it to "none" and the repaired file is
+written back to flash. A node that rolls back from one channel to the other
+therefore loses that slot permanently. Byte equality also catches URL, CA and
+credential drift, which a name-only check passes silently even though the two
+channels are meant to dial the same brokers.
+
+Channel-specific behaviour belongs in MQTTPresetPolicy.h, which is not compared,
+so this file can stay identical while the channels differ elsewhere.
+
+Without ``--exact`` only the first string field of each ``MQTT_PRESETS`` entry
+is compared (set equality, case-sensitive). That is the older, weaker check,
+kept for ad-hoc use.
 
 Usage::
 
-    python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B \\
+    python3 scripts/check_mqtt_preset_parity.py FILE_A FILE_B --exact \\
         --label-a observer-firmware --label-b observer-firmware-dev
 
     python3 scripts/check_mqtt_preset_parity.py --self-test
@@ -137,6 +149,49 @@ def compare(
     ]
 
 
+def compare_exact(
+    path_a: Path,
+    path_b: Path,
+    *,
+    label_a: str,
+    label_b: str,
+) -> list[str]:
+    """Return error lines if the two files differ byte for byte."""
+
+    data_a = path_a.read_bytes()
+    data_b = path_b.read_bytes()
+    if data_a == data_b:
+        return []
+
+    import difflib
+
+    errors = [f"{label_a} and {label_b} do not match byte for byte."]
+    try:
+        diff = list(
+            difflib.unified_diff(
+                data_a.decode("utf-8").splitlines(),
+                data_b.decode("utf-8").splitlines(),
+                fromfile=label_a,
+                tofile=label_b,
+                lineterm="",
+                n=1,
+            )
+        )
+    except UnicodeDecodeError:
+        errors.append("(binary difference; cannot render a text diff)")
+        return errors
+
+    # Enough to identify the drift without pasting the whole table into a log.
+    errors.extend(diff[:60])
+    if len(diff) > 60:
+        errors.append(f"... {len(diff) - 60} more diff line(s)")
+    errors.append(
+        "Preset rows must be identical on both channels. Behaviour that differs "
+        "per channel belongs in MQTTPresetPolicy.h."
+    )
+    return errors
+
+
 def load_names(path: Path) -> set[str]:
     text = path.read_text(encoding="utf-8")
     names, _ = parse_presets(text, source=str(path))
@@ -237,6 +292,26 @@ static const MQTTPresetDef MQTT_PRESETS[MQTT_PRESET_COUNT] = {{
             print("self-test failed: URL scheme was treated as a comment.", file=sys.stderr)
             return 1
 
+        # --exact: identical bytes pass, any drift fails — including a change
+        # that a name-only comparison waves through.
+        if compare_exact(equal_a, equal_a, label_a="a", label_b="a-copy"):
+            print("self-test failed: a file was not equal to itself.", file=sys.stderr)
+            return 1
+        if compare(load_names(equal_a), load_names(equal_b), label_a="a", label_b="b"):
+            print("self-test failed: fixture assumption broken.", file=sys.stderr)
+            return 1
+        exact_errs = compare_exact(equal_a, equal_b, label_a="a", label_b="b")
+        if not exact_errs or "byte for byte" not in exact_errs[0]:
+            print(
+                "self-test failed: --exact accepted files that differ only in "
+                f"config: {exact_errs!r}",
+                file=sys.stderr,
+            )
+            return 1
+        if not any("MQTTPresetPolicy.h" in line for line in exact_errs):
+            print("self-test failed: --exact failure omitted the remedy.", file=sys.stderr)
+            return 1
+
         # Silence unused path in fixture layout.
         dupes.write_text("// unused\n", encoding="utf-8")
 
@@ -250,6 +325,11 @@ def main(argv: list[str] | None = None) -> int:
     parser.add_argument("file_b", type=Path, nargs="?", help="Second MQTTPresets.h (e.g. dev)")
     parser.add_argument("--label-a", default="file_a", help="Label for file_a in reports")
     parser.add_argument("--label-b", default="file_b", help="Label for file_b in reports")
+    parser.add_argument(
+        "--exact",
+        action="store_true",
+        help="Require the two files to be byte-identical (what CI enforces)",
+    )
     parser.add_argument("--self-test", action="store_true")
     args = parser.parse_args(argv)
 
@@ -259,6 +339,23 @@ def main(argv: list[str] | None = None) -> int:
     if args.file_a is None or args.file_b is None:
         parser.error("FILE_A and FILE_B are required unless --self-test is set")
 
+    if args.exact:
+        try:
+            errors = compare_exact(
+                args.file_a, args.file_b, label_a=args.label_a, label_b=args.label_b
+            )
+        except OSError as error:
+            print(f"MQTT preset parity check could not run: {error}", file=sys.stderr)
+            return 2
+        if errors:
+            print("MQTT preset parity check failed:", *errors, sep="\n  ", file=sys.stderr)
+            return 1
+        print(
+            f"MQTT preset parity check passed: {args.label_a} and {args.label_b} "
+            "have identical preset tables."
+        )
+        return 0
+
     try:
         names_a = load_names(args.file_a)
         names_b = load_names(args.file_b)

src/helpers/MQTTPresetPolicy.h

@@ -0,0 +1,34 @@
+#pragma once
+
+#include "MQTTPresets.h"
+
+#include <string.h>
+
+// Policy that keys off a preset rather than describing one.
+//
+// MQTTPresets.h is data: it is the broker table, and it is kept byte-identical
+// between the observer-firmware and observer-firmware-dev channels so a node
+// that rolls back cannot meet a preset name its firmware does not know. An
+// unknown name is not merely ignored — MQTTPrefsSerializer repairs it to "none"
+// and the repaired /mqtt.json is written back, so the operator's slot is gone
+// for good. Channel-specific behaviour therefore lives here instead, where the
+// two channels are free to differ.
+
+// True when the broker tears down a live session once its JWT passes exp, so the
+// renewal must proactively bounce the connection to present a fresh token.
+//
+// Default true, because getting this wrong the safe way costs a re-handshake and
+// getting it wrong the unsafe way costs an outage. waev is the exception: its
+// operator confirmed (2026-08-11) that their servers do not disconnect on expiry,
+// so a live session there needs only its credentials refreshed for the next
+// reconnect. waev is also the only preset with a short token_lifetime, so it was
+// the only one bouncing often — every ~47 min, and each bounce's re-handshake can
+// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board.
+//
+// Keyed by name rather than a struct field on purpose: adding a field would mean
+// re-ordering a dozen positional initialisers in the table, where a mistake is
+// silent.
+static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) {
+  if (!preset || !preset->name) return true;   // custom/audience slots: assume enforced
+  return strcmp(preset->name, "waev") != 0;
+}

src/helpers/MQTTPresets.h

@@ -47,24 +47,6 @@ struct MQTTPresetDef {
 // Braces match topic placeholders ({device}/{iata}); never send this string to the broker.
 static const char MQTT_USERPASS_USERNAME_PUBKEY[] = "{pubkey}";
 
-// True when the broker tears down a live session once its JWT passes exp, so the
-// renewal must proactively bounce the connection to present a fresh token.
-//
-// Default true, because getting this wrong the safe way costs a re-handshake and
-// getting it wrong the unsafe way costs an outage. waev is the exception: its
-// operator confirmed (2026-08-11) that their servers do not disconnect on expiry,
-// so a live session there needs only its credentials refreshed for the next
-// reconnect. waev is also the only preset with a short token_lifetime, so it was
-// the only one bouncing often — every ~47 min, and each bounce's re-handshake can
-// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board.
-//
-// Keyed by name rather than a struct field on purpose: adding a field would mean
-// re-ordering a dozen positional initialisers below, where a mistake is silent.
-static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) {
-  if (!preset || !preset->name) return true;   // custom/audience slots: assume enforced
-  return strcmp(preset->name, "waev") != 0;
-}
-
 static inline bool mqttPresetUsesDevicePubkeyUsername(const MQTTPresetDef* preset) {
   return preset && preset->auth_type == MQTT_AUTH_USERPASS &&
          preset->userpass_username &&

src/helpers/bridges/MQTTBridge.h

@@ -10,6 +10,7 @@
 #include "helpers/JWTHelper.h"
 #include "helpers/MQTTPacketFilter.h"
 #include "helpers/MQTTPresets.h"
+#include "helpers/MQTTPresetPolicy.h"
 #include "helpers/MQTTLifecycle.h"
 #include "helpers/AlertFaultPolicy.h"
 #include "helpers/MQTTEffectiveConfig.h"