whoop5: guard the failed flag read on every platform, and on Android too

Packages/WhoopProtocol/Sources/WhoopProtocol/DeviceConfigReadProbe.swift

@@ -472,13 +472,16 @@ public struct DeviceConfigReadProbeReport: Equatable, Sendable {
     /// Record one decoded reply.
     public mutating func noteReply(_ r: DeviceConfigReadProbe.ValueResponse, for step: Step) {
         setStatus(r.isUnsupported ? .unsupported : .answered, for: step.opcode)
-        #if os(macOS)
-        // The macOS hardware run returned FAILURE with an echoed key and zero padding. Keep the
-        // low-level cross-platform decoder intact; this scoped report must not claim a failed value.
+        // The hardware run that found this was on macOS, but the fault is not: a FAILURE reply echoes
+        // the requested key back with zero padding, so taking `value(for:)` regardless of the result
+        // code renders a rejected read as a stored 0, indistinguishable from a key that holds 0. This
+        // file has no platform gate and builds for iOS too, so scoping the guard left the bug live
+        // there, with the test that catches it compiled out by the same condition. A report that
+        // fabricates a value is wrong wherever it runs. (#2193)
+        //
+        // The low-level cross-platform decoder is untouched: `value(for:)` still answers what the
+        // bytes say. This is the report declining to claim it.
         let value = r.resultCode == nil || r.resultCode == 1 ? r.value(for: step.key) : nil
-        #else
-        let value = r.value(for: step.key)
-        #endif
         readings.append(Reading(group: step.group, opcode: step.opcode, key: step.key, value: value,
                                 resultCode: r.resultCode, recordHex: r.recordHex))
         var line = "\(DeviceConfigReadProbeReport.opcodeLabel(step.opcode)) key=\"\(step.key)\""
@@ -566,14 +569,14 @@ public struct DeviceConfigReadProbeReport: Equatable, Sendable {
         }
         let named = readings.filter { $0.value != nil }.count
         if named == 0 {
-            #if os(macOS)
+            // Same reasoning as the guard above: the verdict has to distinguish "every reply was
+            // rejected" from "replies succeeded but none carried a verified pair", on every platform.
+            // The old `#else` said only the second, which is the wrong sentence for a run where the
+            // strap refused every read. (#2193)
             if readings.allSatisfy({ $0.resultCode != nil && $0.resultCode != 1 }) {
                 return "\(answered) of 2 read verbs answered, but no reply reported success; no value is claimed"
             }
             return "\(answered) of 2 read verbs answered, but no successful reply carried a verified key/value pair; no value is claimed"
-            #else
-            return "\(answered) of 2 read verbs answered, but no reply echoed its key so no value is claimed"
-            #endif
         }
         return "\(answered) of 2 read verbs answered; read \(named) config value(s)"
     }

Packages/WhoopProtocol/Tests/WhoopProtocolTests/DeviceConfigReadProbeTests.swift

@@ -159,7 +159,10 @@ final class DeviceConfigReadProbeTests: XCTestCase {
         XCTAssertNil(r.value(for: "whatever"), "an UNSUPPORTED reply must never yield a value")
     }
 
-    #if os(macOS)
+    // These were gated to macOS because that is where the hardware run happened. The behaviour they
+    // pin is not macOS-specific, and gating them meant the guard-only test could not compile on the
+    // platform where the guard was missing: `Executed 0 tests` off macOS, against a probe that builds
+    // for iOS. A test that only runs where the bug is already fixed cannot catch the bug. (#2193)
     func testEchoedFailureBytesAreNotReportedAsStoredValues() {
         // The live WHOOP 5 oxygen-key reads returned FAILURE with the requested key and zeroes.
         for result in [UInt8(0), 2, 3] {
@@ -222,7 +225,6 @@ final class DeviceConfigReadProbeTests: XCTestCase {
         XCTAssertEqual(succeeded.readings.first?.value, 0, "a SUCCESS reply holding 0 is still a real 0")
         XCTAssertTrue(succeeded.render().contains("value=0x00"))
     }
-    #endif
 
     func testNoValueIsClaimedWhenTheReplyDoesNotEchoTheKey() {
         // A plausible-looking record that simply isn't the key we asked for.

android/app/src/main/java/com/noop/protocol/DeviceConfigReadProbe.kt

@@ -422,7 +422,14 @@ class DeviceConfigReadProbeReport(
     /** Record one decoded reply. */
     fun noteReply(r: DeviceConfigReadProbe.ValueResponse, step: Step) {
         setStatus(if (r.isUnsupported) VerbStatus.UNSUPPORTED else VerbStatus.ANSWERED, step.opcode)
-        val value = r.valueFor(step.key)
+        // Twin of the Swift guard (#2193, @Trillient). A FAILURE reply echoes the requested key back
+        // with zero padding, so taking valueFor() regardless of the result code renders a rejected read
+        // as a stored 0, indistinguishable from a key that genuinely holds 0. Found on a WHOOP 5 where
+        // eight guessed keys came back FAILURE and every one of them was reported as a value.
+        //
+        // valueFor() itself is untouched and still answers what the bytes say; this is the report
+        // declining to claim it.
+        val value = if (r.resultCode == null || r.resultCode == 1) r.valueFor(step.key) else null
         _readings.add(
             Reading(step.group, step.opcode, step.key, value, r.resultCode, r.recordHex),
         )
@@ -522,7 +529,13 @@ class DeviceConfigReadProbeReport(
             }
             val named = _readings.count { it.value != null }
             if (named == 0) {
-                return "$answered of 2 read verbs answered, but no reply echoed its key so no value is claimed"
+                // Two sentences, matching Swift: "every reply was rejected" and "replies succeeded but
+                // none carried a verified pair" are different findings, and the single sentence said
+                // only the second, which is the wrong one for a strap that refused every read.
+                if (_readings.all { it.resultCode != null && it.resultCode != 1 }) {
+                    return "$answered of 2 read verbs answered, but no reply reported success; no value is claimed"
+                }
+                return "$answered of 2 read verbs answered, but no successful reply carried a verified key/value pair; no value is claimed"
             }
             return "$answered of 2 read verbs answered; read $named config value(s)"
         }

android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt

@@ -575,4 +575,87 @@ class DeviceConfigReadProbeTest {
             "  GET_FF_VALUE(128) key=\"enable_spo2\" → result=FAILURE(0) record=[01 00]\n"
         assertEquals(golden, rep.render())
     }
+
+    /**
+     * Twin of the Swift guard test (#2193). A FAILURE reply echoes the key back with zero padding, so
+     * the report must not present it as a stored 0.
+     *
+     * Built the same way round as the Swift one, which is what makes it a guard test rather than a
+     * shape test: it first asserts the fixture really is the dangerous frame, then asserts the report
+     * declines it, and a SUCCESS reply carrying the identical record still reports a real 0 so the test
+     * cannot pass by the report having stopped rendering values at all.
+     */
+    @Test
+    fun aFailedReadIsNotReportedAsAStoredValue() {
+        val record = echoRecord("enable_rocky2", 0)
+        fun report(result: Int): DeviceConfigReadProbeReport {
+            val frame = whoop5Response(128, payload(result, record))
+            val reply = DeviceConfigReadProbe.parse(frame, DeviceFamily.WHOOP5, 128).value
+            assertNotNull("the response is valid framing even when the read failed", reply)
+            reply!!
+            assertEquals(
+                "the fixture must be the dangerous shape: key echoed, zero byte after the field",
+                0, reply.valueFor("enable_rocky2"),
+            )
+            val out = DeviceConfigReadProbeReport(DeviceFamily.WHOOP5, emptyList(), emptyList())
+            out.noteReply(reply, DeviceConfigReadProbeReport.Step(128, "enable_rocky2", DeviceConfigReadProbeReport.Group.KNOWN_FLAG))
+            return out
+        }
+
+        val failed = report(0)
+        assertEquals("the rejected read is still recorded", 1, failed.readings.size)
+        assertEquals(0, failed.readings.first().resultCode)
+        assertNull("a FAILURE reply must not be reported as a stored 0", failed.readings.first().value)
+        assertFalse(failed.render().contains("value="))
+
+        val succeeded = report(1)
+        assertEquals("a SUCCESS reply holding 0 is still a real 0", 0, succeeded.readings.first().value)
+        assertTrue(succeeded.render().contains("value="))
+    }
+
+    /**
+     * Twin of the Swift verdict assertion, which Kotlin was missing entirely.
+     *
+     * The verdict has to separate "every reply was rejected" from "replies succeeded but none carried a
+     * verified key/value pair". Kotlin said only the second, and no Kotlin test asserted the sentence at
+     * all, so changing it failed nothing. Swift pinned it and Kotlin did not, which is how the two
+     * drifted in the first place.
+     */
+    @Test
+    fun theVerdictSaysWhenEveryReplyWasRejectedRatherThanUnverified() {
+        for (result in listOf(0, 2)) {
+            val frame = whoop5Response(121, payload(result, echoRecord("enable_spo2", 0)))
+            val reply = DeviceConfigReadProbe.parse(frame, DeviceFamily.WHOOP5, 121).value
+            assertNotNull(reply)
+            val rep = DeviceConfigReadProbeReport(DeviceFamily.WHOOP5, emptyList(), emptyList())
+            rep.noteReply(reply!!, DeviceConfigReadProbeReport.Step(121, "enable_spo2", DeviceConfigReadProbeReport.Group.CANDIDATE))
+            assertEquals("the shared byte decoder remains unchanged", 0, reply.valueFor("enable_spo2"))
+            assertNull(rep.readings.first().value)
+            assertEquals(
+                "1 of 2 read verbs answered, but no reply reported success; no value is claimed",
+                rep.verdict,
+            )
+        }
+    }
+
+    /**
+     * The OTHER branch of the two-sentence verdict, and the one I left unpinned on this side while
+     * pinning its twin. Swift has both; Kotlin had neither until now.
+     *
+     * A reply that SUCCEEDED but carried no verified key/value pair is a different finding from one
+     * that was rejected outright, and the verdict has to say which. Found by listing both suites and
+     * comparing them rather than by reading the code again.
+     */
+    @Test
+    fun aSuccessfulReplyWithoutAKeyValueHasADistinctVerdict() {
+        val rep = DeviceConfigReadProbeReport(DeviceFamily.WHOOP5, emptyList(), emptyList())
+        rep.noteReply(
+            DeviceConfigReadProbe.ValueResponse(1, byteArrayOf(1, 0)),
+            DeviceConfigReadProbeReport.Step(128, "enable_r22_packets", DeviceConfigReadProbeReport.Group.DISCOVERY),
+        )
+        assertEquals(
+            "1 of 2 read verbs answered, but no successful reply carried a verified key/value pair; no value is claimed",
+            rep.verdict,
+        )
+    }
 }