fix: do not report a failed WHOOP 5 flag read as a value - #2193
Conversation
`DeviceConfigReadProbe` built its report from `r.value(for:)` regardless of the reply's result code. A FAILURE reply echoes the requested key back with zero padding, so a rejected read was rendered as a stored value of 0 — indistinguishable from a key that genuinely holds 0. Take a value only when the reply reported success (or carried no result code), and say plainly in the report when verbs answered but no reply succeeded, rather than presenting a fabricated zero. Also read four additional WHOOP 5 flag names returned by a complete 117/118 enumeration on firmware 50.41.1.0: enable_r22_v9_packets, enable_frizzle_burst_mode, ir_1x_enable and enable_rocky2. All four answered CRC-valid SUCCESS with ASCII '2' across 29 replies (21 successful, eight rejected guessed keys), with no timeout or reconnect. Their effects and accepted values are unknown, so they are read-only additions: the R22 write sequence is untouched, and the report title distinguishes enumerated names from names NOOP itself writes. Scoped to macOS, where the hardware evidence was taken; iOS and Android keep their existing probe plans and decoding.
|
Reviewed at 6cf7fe4. The bug is real and the fix is right: a FAILURE reply echoes the key back with zero padding, so reporting Taking you up on the offer. You wrote that the negative control is weaker here, that reverting The guard is the fix; the four enumerated names are additive and would survive a silent revert of it. A test that constructs a FAILURE reply with the key echoed and zero padding, and asserts the report does not present a value, is the one that fails if someone later simplifies the result-code check away. Please add it. The read-only framing on the four new names is the right call as well, given their effects and accepted values are unknown, and keeping the R22 write sequence untouched is what makes that safe. Distinguishing enumerated names from names NOOP itself writes, in the report title, is a detail I would not have thought to ask for and would have missed. 15 of 15 checks. Happy to merge once the guard test is in. |
|
Some additional context from our WHOOP 5 protocol work:
The value Related documentation: |
Add a guard-only regression test for the result-code check in DeviceConfigReadProbeReport.noteReply. A CRC-valid WHOOP 5 GET_FF_VALUE(128) FAILURE reply echoing the key with a zero byte must not produce a reading value or a rendered value=, while a SUCCESS reply with the identical record still reports a genuine 0. The test uses only API present on main, so removing the check fails an assertion rather than the build.
|
@ryanbr Added It builds a CRC-valid WHOOP 5 GET_FF_VALUE(128) reply with result FAILURE, the key echoed in the 32-byte field and a zero byte after it. First it asserts that the shared decoder would read that as 0, so the fixture really is the dangerous shape. Then it passes the reply through the report and asserts that the reading carries no value, that the FAILURE result code is kept, and that the rendered report contains no Negative controls, run rather than argued:
@bhelm Thanks, agreed. The report doesn't interpret the value: it prints the raw stored byte, |
|
Reviewed at 75668ee. The test is exactly what I asked for and better built than I expected. It also cannot do its job yet, for a reason that answers your design question outright. The test is rightThe two things that make it a guard test rather than a shape test are both there. It asserts the fixture is dangerous before using it, Your negative control is the right one too, and run rather than argued. It is inside the
|
…too (#2223) Follow-up to #2193 by @Trillient, which stopped the report claiming a value from a FAILURE reply. Two things were left, and the second was not visible from that PR at all. @Trillient asked whether the guard belonged inside `#if os(macOS)`, judged that it did not, and left the call here. 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 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 what it was written for. Both are out of the `#if`, which brings three formerly-gated tests into the suite everywhere, 707 to 710. The verdict came 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. Then the part found by grepping for the string being deleted: Android has a DeviceConfigReadProbe twin, #2193 correctly declared it did not touch android/, and nobody had asked whether the twin shared the fault. It did. val value = r.valueFor(step.key) No result-code check, exactly what Swift had before, live on every Android device and entirely outside the Swift fix. Guarded now, with the same two sentences and a Kotlin twin of the guard test. Two further gaps came out of re-reviewing this rather than the code. Changing the Kotlin verdict passed the suite, because Kotlin asserted neither sentence while Swift asserted both, which is how they drifted in the first place. Pinning the rejection branch then left the other branch unpinned, found by listing both suites and comparing them rather than by reading again. Every Swift probe test now has a Kotlin counterpart except ObservedWhoop5FlagsAreReadWithoutExtendingTheWriteSequence, covering the four enumerated key names. Those stay macOS-scoped: observations from one firmware on one platform, which is @Trillient's own distinction, and @bhelm notes enable_rocky2 is absent from their 50.42.1.0 inventory, which carries enable_rocky_again instead. Negative controls run on both sides. Removing the Swift guard fails three assertions ON LINUX, impossible before this change. Removing the Kotlin guard fails aFailedReadIsNotReportedAsAStoredValue. Reverting the Kotlin verdict fails theVerdictSaysWhenEveryReplyWasRejectedRatherThanUnverified, which before this failed nothing. Every file restored byte-for-byte, verified by grep. WhoopProtocol 710 tests 1 skipped 0 failures. com.noop.protocol 576 tests 0 failures. Parity ledger reports no new findings, and com/noop/protocol is not in the excluded globs, so it genuinely scanned these files.
3 participants
What this PR does
DeviceConfigReadProbebuilt its report fromr.value(for:)regardless of the reply's result code. A FAILURE reply echoes the requested key back with zero padding, so a rejected read was rendered as a stored value of0— indistinguishable in the report from a key that genuinely holds0.Take a value only when the reply reported success (or carried no result code), and say plainly when read verbs answered but no reply succeeded, instead of presenting a fabricated zero.
Secondly, read four additional WHOOP 5 flag names returned by a complete 117/118 enumeration on firmware 50.41.1.0:
enable_r22_v9_packets,enable_frizzle_burst_mode,ir_1x_enable,enable_rocky2. Their effects and accepted values are unknown, so these are read-only additions — the R22 write sequence is untouched, and the report title distinguishes enumerated names from names NOOP itself writes.Type of change
How it was tested
'2'. The eight rejected keys are what exposed the bug: each came back FAILURE with the key echoed and zero padding, which the old code reported as a value of0.swift testinPackages/WhoopProtocol: 707 tests, 1 skipped, 0 failures.DeviceConfigReadProbe.swifttomainmakesDeviceConfigReadProbeTestsfail to compile (the new tests referenceknownFlagKeys), rather than fail assertions. So it demonstrates the tests exercise the new surface, not that they'd catch a silent regression of the guard alone. If you want a guard-only regression test that survives without the enumeration change, I'll add one.Checklist
swift testinPackages/WhoopProtocol)android/— not touchedStrandDesigntokens — no UI changesdocs/CONTRIBUTING.mdStrand.xcodeproj/) or any secrets/keystoresOne design question for you
Both parts are currently behind
#if os(macOS), because that's where the hardware evidence was taken.That's clearly right for the four enumerated key names — they're observations from one firmware on one platform. I'm less sure it's right for the FAILURE guard: reporting a rejected read as a value of
0looks like a plain correctness bug rather than a platform-specific one, and the argument for fixing it everywhere seems stronger than the argument for scoping it. I left it scoped to keep the change conservative and match where the evidence came from, but I'd rather you decide — I'm happy to lift the guard out of the#ifin this PR.Related issues
None.