whoop5: guard the failed flag read on every platform, and on Android too - #2223
Merged
Merged
Conversation
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2193 by @Trillient. Two things were left after that merged, and the second I went looking for only because a grep turned up a string in an unexpected file.
The scoping question @Trillient asked, answered
They asked whether the FAILURE guard belonged inside
#if os(macOS), judged that it did not, and left the call to me. They were right, and the evidence turned out sharper than the argument.DeviceConfigReadProbehas no platform gate. It builds for iOS, and there the#elsebranch still did:So a rejected read was still rendered as a stored
0on iOS. Worse, the guard-only test they wrote sat inside the same#if, so it could not compile on the platform where the bug survived:A guard test that only runs where the guard already applies cannot catch the thing it was written for. Both are out of the
#ifnow, which brings three formerly-gated tests into the suite everywhere: 707 to 710.The verdict string 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.
Android had the same bug, untouched
Grepping for the removed non-macOS verdict string found it in
android/app/src/main/java/com/noop/protocol/DeviceConfigReadProbe.kt. There is a Kotlin twin, #2193 correctly said it did not touchandroid/, and nobody had looked at whether the twin shared the fault.It did:
No result-code check. Exactly what Swift had before #2193, live on every Android device, and not covered by the Swift fix at all.
Guarded now, with the same two-sentence verdict, plus a Kotlin twin of the guard test built the same way round as @Trillient's: assert the fixture really is the dangerous frame first, then that the report declines it, with a SUCCESS reply carrying the identical record still reporting a real
0so the test cannot pass by the report having stopped rendering values.Negative controls, run on both sides
aFailedReadIsNotReportedAsAStoredValue.Both files restored byte-for-byte afterwards, verified by grep rather than by assumption.
Verification
WhoopProtocol: 710 tests, 1 skipped, 0 failures.com.noop.protocol: 575 tests, 0 failures.compileFullDebugKotlinanddoc_comment_lintclean.Unchanged on purpose
The four enumerated key names stay macOS-scoped. Those are observations from one firmware on one platform, which is exactly the distinction @Trillient drew, and @bhelm's note that
enable_rocky2is absent from their 50.42.1.0 inventory is a second argument for keeping them scoped and version-bound.