From 4308798063465ccacc73f97aa6173525f1f7d570 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Tue, 15 Sep 2026 13:56:17 +1200 Subject: [PATCH 1/3] whoop5: guard the failed flag read on every platform, and on Android too Follow-up to #2193 by @Trillient, which stopped the report claiming a value from a FAILURE reply. Two things were left, and the second one I did not expect. @Trillient asked whether the guard belonged inside `#if os(macOS)`, judged that it did not, and left the call to me. They were right, and the evidence is sharper than the argument: DeviceConfigReadProbe has no platform gate and builds for iOS, so the `#else` branch still rendered a rejected read as a stored 0 there. The guard-only test they wrote sat inside the same `#if`, so it could not compile on the platform where the bug survived. `Executed 0 tests` off macOS. A guard test that only runs where the guard already applies cannot catch the thing it was written for. Both are out of the `#if` now: three formerly-gated tests join the suite, 707 to 710, and the guard applies wherever the probe does. The verdict came out with it. The non-macOS branch said only "no reply echoed its key", which is the wrong sentence for a strap that answered and refused every read; both platforms now distinguish that from "replies succeeded but none carried a verified pair". Then the part I went looking for because the removed string turned up in a grep: Android has a DeviceConfigReadProbe twin, it was never touched, and it had the identical bug. val value = r.valueFor(step.key) No result-code check, exactly what Swift had before #2193, live on every Android device. Guarded now, with the same two-sentence verdict, and a Kotlin twin of the guard test built the same way round: assert the fixture is the dangerous frame first, then that the report declines it, with a SUCCESS reply carrying the identical record still reporting a real 0 so it cannot pass by the report having stopped rendering values. Negative controls, run on both sides rather than argued. Removing the Swift guard now fails three assertions ON LINUX, which was impossible before this change because the tests did not compile there. Removing the Kotlin guard fails aFailedReadIsNotReportedAsAStoredValue. Both files restored byte-for-byte. WhoopProtocol 710 tests 1 skipped 0 failures. com.noop.protocol 575 tests 0 failures. The four enumerated key names stay macOS-scoped: those are observations from one firmware on one platform, which is the distinction @Trillient drew and it is the right one. --- .../WhoopProtocol/DeviceConfigReadProbe.swift | 23 +++++++----- .../DeviceConfigReadProbeTests.swift | 6 ++- .../noop/protocol/DeviceConfigReadProbe.kt | 17 ++++++++- .../protocol/DeviceConfigReadProbeTest.kt | 37 +++++++++++++++++++ 4 files changed, 69 insertions(+), 14 deletions(-) diff --git a/Packages/WhoopProtocol/Sources/WhoopProtocol/DeviceConfigReadProbe.swift b/Packages/WhoopProtocol/Sources/WhoopProtocol/DeviceConfigReadProbe.swift index 3553019be9..b6b148d0a0 100644 --- a/Packages/WhoopProtocol/Sources/WhoopProtocol/DeviceConfigReadProbe.swift +++ b/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)" } diff --git a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DeviceConfigReadProbeTests.swift b/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DeviceConfigReadProbeTests.swift index 38384548ef..f2e727a410 100644 --- a/Packages/WhoopProtocol/Tests/WhoopProtocolTests/DeviceConfigReadProbeTests.swift +++ b/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. diff --git a/android/app/src/main/java/com/noop/protocol/DeviceConfigReadProbe.kt b/android/app/src/main/java/com/noop/protocol/DeviceConfigReadProbe.kt index 14042468ee..559b9f2827 100644 --- a/android/app/src/main/java/com/noop/protocol/DeviceConfigReadProbe.kt +++ b/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)" } diff --git a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt index 91e7c20be8..c55a27bacb 100644 --- a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt +++ b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt @@ -575,4 +575,41 @@ 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=")) + } } From 0b1a1912b67cc30ea79fc31833bfe865b84a912d Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Tue, 15 Sep 2026 14:11:39 +1200 Subject: [PATCH 2/3] test(android): pin the verdict Kotlin was silently free to change Re-review of my own PR. When I changed the Kotlin verdict to match Swift's two sentences, the suite passed. It passed because nothing asserted either sentence: Swift pins "no reply reported success" and Kotlin pinned it zero times. So the sentence I had just written was unpinned, and so was the one I replaced. That is how the two platforms drifted apart in the first place, and I had reproduced the condition rather than closed it while fixing the drift. Kotlin now has the twin of Swift's assertion, driving result codes 0 and 2 and checking the shared decoder still reads the byte, the report still declines the value, and the verdict names the rejection rather than an unverified pair. Control: putting the old single-sentence verdict back fails theVerdictSaysWhenEveryReplyWasRejectedRatherThanUnverified. Before this commit it failed nothing, which was the problem. com.noop.protocol 576 tests 0 failures. Also checked this pass and correct as written: Kotlin's result codes match Swift's exactly, 0 FAILURE, 1 SUCCESS, 2 PENDING, 3 UNSUPPORTED, so guarding on `resultCode == 1` means the same thing on both sides. PENDING is deliberately not a value either. The parity ledger reports no new findings. --- .../protocol/DeviceConfigReadProbeTest.kt | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt index c55a27bacb..4870b92602 100644 --- a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt +++ b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt @@ -612,4 +612,29 @@ class DeviceConfigReadProbeTest { 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, + ) + } + } } From 739759d347f41338cf0af30e6787361e8db528bf Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Tue, 15 Sep 2026 14:17:39 +1200 Subject: [PATCH 3/3] test(android): pin the other half of the verdict too Third re-review pass, and this one came from listing both suites and comparing them rather than reading the code again. The verdict has two branches. Last commit pinned the rejection one on Kotlin. Swift pins both, and I had left the second unpinned on the side I was fixing: a reply that SUCCEEDED but carried no verified key/value pair is a different finding from one rejected outright, and only Swift said so under test. So having just written a two-sentence verdict for Kotlin, I had tested one sentence of it. That is a smaller version of the thing this whole PR is about. With this, every Swift probe test has a Kotlin counterpart except ObservedWhoop5FlagsAreReadWithoutExtendingTheWriteSequence, which covers the four enumerated key names. Those stay macOS-scoped because they are observations from one firmware on one platform, so its absence here is correct rather than another gap. Two more pair under clearer Kotlin names: aFailedReadIsNotReportedAsAStoredValue and theVerdictSaysWhenEveryReplyWasRejectedRatherThanUnverified. Kotlin probe suite: 31 tests, 0 failures. --- .../protocol/DeviceConfigReadProbeTest.kt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt index 4870b92602..b41147747a 100644 --- a/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt +++ b/android/app/src/test/java/com/noop/protocol/DeviceConfigReadProbeTest.kt @@ -637,4 +637,25 @@ class DeviceConfigReadProbeTest { ) } } + + /** + * 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, + ) + } }