protocol safety bits: forceTrim comment, gen5 dangerous opcodes, bad frame rev

lib/src/commands.dart

@@ -159,7 +159,7 @@ Uint8List cmdToggleHr(int seq, bool on) =>
 ///
 /// NOTE: sending this with payload `[0x00]` (i.e. `cmdSendR10R11(seq, false)`)
 /// is the REAL persistent raw-flood OFF-switch — the off state persists across
-/// reconnects. STOP_RAW_DATA (0x52) does nothing. (PROTOCOL_FINDINGS.md:168-169)
+/// reconnects. STOP_RAW_DATA (0x52) does nothing.
 Uint8List cmdSendR10R11(int seq, bool on) =>
     buildCommand(seq, Cmd.sendR10R11Realtime, [on ? 0x01 : 0x00]);
 Uint8List cmdToggleImu(int seq, bool on) =>

lib/src/constants.dart

@@ -35,7 +35,7 @@ class PacketType {
 class Cmd {
   static const int linkValid = 0x01;
   // Report the highest wire-protocol revision the strap understands. Response
-  // carries the max protocol version — used for firmware/feature gating.
+  // carries the max protocol version — used for feature gating.
   static const int getMaxProtocolVersion = 0x02;
   static const int toggleRealtimeHr = 0x03;
   static const int reportVersionInfo = 0x07;
@@ -45,8 +45,10 @@ class Cmd {
   static const int abortHistoricalTransmits = 0x14;
   static const int sendHistoricalData = 0x16;
   static const int historicalDataResult = 0x17; // the batch ACK
-  // DANGER — never send. Body is NOT empty: two LE i32 range args
-  // (full erase = both 0xFEFEFEFE). See PROTOCOL_FINDINGS.md.
+  // DANGER — never send. Body is NOT empty and is NOT a range/erase: it is an
+  // 8-byte `[u32 trim_page LE][u32 wrap_count LE]` — the
+  // same 8 bytes as the HISTORY_END trim token. It advances the strap's flash
+  // trim pointer, so a wrong value discards unsynced records.
   static const int forceTrim = 0x19;
   static const int getBatteryLevel = 0x1A;
   static const int rebootStrap = 0x1D; // DANGER
@@ -56,12 +58,18 @@ class Cmd {
   static const int setReadPointer = 0x21;
   static const int getDataRange = 0x22;
   static const int getHelloHarvard = 0x23;
-  // Firmware-load opcodes (Cmd opcode space — distinct from PacketType 0x24
-  // COMMAND_RESPONSE, which is inner[0], not a command opcode).
-  // DANGER — never send (per PROTOCOL_FINDINGS.md destructive list).
-  static const int startFirmwareLoad = 0x24; // DANGER
-  static const int loadFirmwareData = 0x25; // DANGER
-  static const int processFirmwareImage = 0x26; // DANGER
+  // Device-update trio (Cmd opcode space — distinct from PacketType
+  // 0x24 COMMAND_RESPONSE, which is inner[0], not a command opcode).
+  // DANGER — never send.
+  //
+  // ⚠ These 0x24-0x26 values are the GEN4-EMPIRICAL numbering and could not
+  // be confirmed on gen5. A gen5 strap does not act on them; the real gen5
+  // device-update trio is 0x8E/0x8F/0x90 (= 142/143/144), which
+  // [dangerousCmds] now also blocks. Keeping the gen4 entries because their
+  // numbering cannot be disproven, not because it is confirmed.
+  static const int startUpdateLoad = 0x24; // DANGER (gen4-empirical)
+  static const int loadUpdateData = 0x25; // DANGER (gen4-empirical)
+  static const int processUpdateImage = 0x26; // DANGER (gen4-empirical)
   static const int sendR10R11Realtime = 0x3F;
   // On-device haptic alarm. SET carries a wall-clock epoch + a haptic waveform
   // pattern (see cmdSetAlarm in commands.dart for the exact, hardware-verified
@@ -112,7 +120,7 @@ class Cmd {
 /// Band-agnostic opcode safety classification, sourced from whoop-rs's
 /// hardware-tested command surface (kept SEPARATE from [dangerousCmds] above,
 /// which is OpenStrap's own, independently-curated gen4 list — the two do not
-/// fully overlap, e.g. this list omits the firmware-load opcodes (0x24-0x26)
+/// fully overlap, e.g. this list omits the device-update opcodes (0x24-0x26)
 /// that [dangerousCmds] already blocks, and adds a few whoop-rs flags ours
 /// didn't have, notably 120/SET_FF_VALUE — see the note on [forbidden] below).
 ///
@@ -146,24 +154,39 @@ class OpcodeSafety {
   };
 
   /// The subset of [forbidden] that is actively destructive (data loss /
-  /// bricking), not merely "don't auto-fire". Opcodes 142-144 have no named
-  /// meaning in either reference codebase — treat as permanently blocked,
-  /// unknown-but-dangerous.
+  /// bricking), not merely "don't auto-fire". Opcodes 142-144 (0x8E-0x90) are
+  /// the confirmed gen5 device-update trio — [dangerousCmds]
+  /// also lists them so they are actually REFUSED, because this set is
+  /// classification-only and self-enforces NOTHING (see the class doc). Treat
+  /// membership here as documentation; gate on [dangerousCmds] to block a send.
   static const Set<int> destructive = {25, 45, 142, 143, 144};
 
   static bool isForbidden(int opcode) => forbidden.contains(opcode);
   static bool isDestructive(int opcode) => destructive.contains(opcode);
 }
 
-/// Commands that can brick the link / burn battery / brick flash. NEVER auto-fire.
+/// Commands that can brick the link / burn battery / brick flash. NEVER
+/// auto-fire. CALLER-ENFORCED: this package builds frames but has no transport
+/// write path, so it does not itself block a send — a call site (edge, at the
+/// point it writes a command) is expected to check membership here and refuse.
+/// This is the set such a guard should gate on (contrast [OpcodeSafety], which
+/// only sub-classifies and is likewise not self-enforcing).
 const Set<int> dangerousCmds = {
   Cmd.forceTrim,
   Cmd.togglePersistentR21,
   Cmd.rebootStrap,
   Cmd.powerCycleStrap,
-  Cmd.startFirmwareLoad,
-  Cmd.loadFirmwareData,
-  Cmd.processFirmwareImage,
+  // gen4-empirical device-update numbering (unverified; inert on gen5).
+  Cmd.startUpdateLoad,
+  Cmd.loadUpdateData,
+  Cmd.processUpdateImage,
+  // CONFIRMED gen5 device-update trio (0x8E/0x8F/0x90 =
+  // 142/143/144). These are the opcodes a real gen5 strap actually acts on, so
+  // they must be in the enforced set — not merely classified in
+  // [OpcodeSafety.destructive]. No Cmd.* constant: we never construct these.
+  0x8E,
+  0x8F,
+  0x90,
 };
 
 /// Historical-data record type (inner[1] of a 0x2F / data packet).

lib/src/control.dart

@@ -826,6 +826,13 @@ class Decoded {
 /// scale, GET_HELLO opcode, historical-record version family); defaults to
 /// gen4 so every existing caller is unchanged.
 Decoded decodeFrame(Frame frame, {BandProfile profile = BandProfile.gen4}) {
+  // A frame that passed CRC but advertises a frame revision this decoder does
+  // not understand (see [Frame.frameRevOk]) must NOT be decoded with the rev-1
+  // `inner[0]/[1]/[2]` field offsets — surface it instead of silently handing
+  // back a body byte as the packet type / opcode.
+  if (!frame.frameRevOk) {
+    return Decoded('unsupported_frame_rev', {'packet_type': frame.packetType});
+  }
   final inner = frame.inner;
   final pt = frame.packetType;
   try {

lib/src/framing.dart

@@ -23,12 +23,30 @@ class Frame {
   final bool crc8Ok;
   final bool crc32Ok;
 
-  Frame(this.inner, this.crc8Ok, this.crc32Ok);
+  /// Whether the frame header advertises the frame revision this decoder was
+  /// written for (rev-1). The `inner[0]/[1]/[2]` = packetType/seq/opcode field
+  /// offsets below assume rev-1 layout; a rev-2 frame can still pass both CRCs
+  /// yet shift those fields, so [opcode] would silently return a body byte.
+  ///
+  /// gen5 carries an explicit revision byte at header[1] (0x01 on every real
+  /// strap frame — see [BandProfile.buildHeader]); a rev-2 frame stamps 0x02
+  /// there. gen4's 4-byte header has no revision byte, so it is always treated
+  /// as rev-1. Defaults true so directly-constructed frames and the entire
+  /// rev-1 path are unchanged; [parseFrame] sets it false for a gen5 frame
+  /// whose revision byte is not rev-1 — surfacing it instead of mis-decoding.
+  final bool frameRevOk;
+
+  Frame(this.inner, this.crc8Ok, this.crc32Ok, {this.frameRevOk = true});
 
   /// Band-neutral alias for the header-integrity result.
   bool get headerCrcOk => crc8Ok;
 
   bool get valid => crc8Ok && crc32Ok;
+
+  /// Safe to read [packetType]/[seq]/[opcode] with the rev-1 field offsets:
+  /// CRCs pass AND the frame revision is one this decoder understands. Check
+  /// this (not just [valid]) before trusting [opcode] on an inbound frame.
+  bool get decodable => valid && frameRevOk;
   int get packetType => inner.isNotEmpty ? inner[0] : -1;
   int get seq => inner.length > 1 ? inner[1] : -1;
   int get opcode => inner.length > 2 ? inner[2] : -1;
@@ -81,7 +99,13 @@ Frame? parseFrame(Uint8List raw, {BandProfile profile = BandProfile.gen4}) {
   final storedBd =
       raw.buffer.asByteData(raw.offsetInBytes + innerStart + declared - 4, 4);
   final stored = storedBd.getUint32(0, Endian.little);
-  return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner));
+  // gen5 header[1] is the frame-revision byte (rev-1 = 0x01). A rev-2 frame can
+  // pass both CRCs but shifts the inner field offsets, so flag it rather than
+  // let the rev-1 getters return a body byte as the opcode. gen4 has no such
+  // byte and is always rev-1.
+  final frameRevOk = !profile.isGen5 || raw[1] == revision1;
+  return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner),
+      frameRevOk: frameRevOk);
 }
 
 /// Length-based reassembler. feed() returns every complete Frame it can carve

lib/src/gen5_records.dart

@@ -104,7 +104,7 @@ abstract class Gen5HistoricalRecord {
 /// per-field below — several fields have OPEN semantic disagreements between
 /// the two reference implementations (whoop-rs vs noop) that could not be
 /// resolved from bytes alone; those are called out explicitly rather than
-/// silently picking a side. See PROTOCOL_FINDINGS / the multiband spec §1.7.
+/// silently picking a side.
 class Gen5HistorySample extends Gen5HistoricalRecord {
   /// bpm. 0 is a legitimate reading (device warming up), not absence.
   final int heartRate;

test/framing_test.dart

@@ -178,4 +178,80 @@ void main() {
       expect(frames.every((f) => f.valid), isTrue);
     });
   });
+
+  group('dangerousCmds (enforced never-send set)', () {
+    test('includes the confirmed gen5 brick trio 0x8E/0x8F/0x90', () {
+      expect(dangerousCmds.containsAll(<int>{0x8E, 0x8F, 0x90}), isTrue);
+    });
+
+    test('keeps the gen4-empirical device-update trio + destructive singles', () {
+      expect(
+        dangerousCmds,
+        containsAll(<int>[
+          Cmd.startUpdateLoad,
+          Cmd.loadUpdateData,
+          Cmd.processUpdateImage,
+          Cmd.forceTrim,
+          Cmd.rebootStrap,
+          Cmd.powerCycleStrap,
+          Cmd.togglePersistentR21,
+        ]),
+      );
+    });
+
+    test('OpcodeSafety.destructive gen5 opcodes (142-144) are actually enforced', () {
+      final gen5Destructive =
+          OpcodeSafety.destructive.where((o) => o >= 142).toSet();
+      expect(gen5Destructive, {142, 143, 144});
+      expect(dangerousCmds.containsAll(gen5Destructive), isTrue,
+          reason: 'classification-only set must be blocked by the enforced set');
+    });
+  });
+
+  group('frame-rev-2 hardening', () {
+    test('gen5 rev-2 frame passes CRC but is flagged non-decodable', () {
+      final raw = Uint8List.fromList(
+          buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5));
+
+      final rev1 = parseFrame(raw, profile: BandProfile.gen5)!;
+      expect(rev1.valid, isTrue);
+      expect(rev1.frameRevOk, isTrue);
+      expect(rev1.decodable, isTrue);
+
+      // Forge a rev-2 frame: bump the header revision byte (header[1]) and fix
+      // the header crc16 so the header-integrity check still passes — i.e. the
+      // frame stays .valid, exactly the latent case that used to mis-decode.
+      raw[1] = 0x02;
+      final c = crc16Modbus(raw.sublist(0, 6));
+      raw[6] = c & 0xFF;
+      raw[7] = (c >> 8) & 0xFF;
+
+      final rev2 = parseFrame(raw, profile: BandProfile.gen5)!;
+      expect(rev2.valid, isTrue, reason: 'both CRCs still pass');
+      expect(rev2.frameRevOk, isFalse, reason: 'revision byte 0x02 is not rev-1');
+      expect(rev2.decodable, isFalse,
+          reason: 'surfaced, not silently decoded as rev-1');
+    });
+
+    test('gen4 has no revision byte and is always treated as rev-1', () {
+      final f = parseFrame(buildCommand(0, Cmd.getHelloHarvard, const [0x00]))!;
+      expect(f.frameRevOk, isTrue);
+      expect(f.decodable, isTrue);
+    });
+
+    test('decodeFrame surfaces a rev-2 frame instead of decoding rev-1 fields', () {
+      final raw = Uint8List.fromList(
+          buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5));
+      raw[1] = 0x02; // bump frame revision
+      final c = crc16Modbus(raw.sublist(0, 6)); // keep header CRC valid
+      raw[6] = c & 0xFF;
+      raw[7] = (c >> 8) & 0xFF;
+
+      final f = parseFrame(raw, profile: BandProfile.gen5)!;
+      expect(f.valid, isTrue);
+      final d = decodeFrame(f, profile: BandProfile.gen5);
+      expect(d.kind, 'unsupported_frame_rev',
+          reason: 'must not be decoded with rev-1 offsets');
+    });
+  });
 }

test/whoop_protocol_update_test.dart

@@ -102,14 +102,14 @@ void main() {
           Cmd.rebootStrap,
           Cmd.powerCycleStrap,
           Cmd.togglePersistentR21,
-          Cmd.startFirmwareLoad,
-          Cmd.loadFirmwareData,
-          Cmd.processFirmwareImage,
+          Cmd.startUpdateLoad,
+          Cmd.loadUpdateData,
+          Cmd.processUpdateImage,
         ]),
       );
       // 0x24 here is a Cmd opcode; PacketType.commandResponse (0x24) is a
       // separate namespace (inner[0], not inner[2]).
-      expect(Cmd.startFirmwareLoad, 0x24);
+      expect(Cmd.startUpdateLoad, 0x24);
       expect(Cmd.powerCycleStrap, 0x20);
       // 0x62 is GET_EXTENDED_BATTERY_INFO; there is no 0x63 command (the old
       // "reset fuel gauge" opcode was a decode artifact and has been removed).