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..b41147747a 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,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, + ) + } }