Repository navigation
feat(ble): adopt a stable whoop-<serial> identity for 5.0/MG (#1303 phase 1) - #1455
Merged
Merged
Conversation
ryanbr
force-pushed
the
feat/whoop5-serial-identity
branch
from
August 19, 2026 09:30
101dcc8 to
a0dc842
Compare
…hase 1) WHOOP strap identity is a TRANSIENT CoreBluetooth UUID / Bluetooth address, so a re-pair or factory reset mints a fresh one and the same physical strap forks into a second registry row, orphaning its history (#1193). The ring solved this already: adoptSerialIdentity re-points a provisional id onto a serial id and migrates every device-scoped row, and it has been shipping on both platforms since #771. This is the WHOOP half of the same idea - no new migration, the proven one is simply called from the WHOOP path. Scoped to 5.0/MG deliberately. Their serial is ALREADY read at connect from DIS 0x2A25 for MG-vs-5.0 discrimination, so this adds no BLE traffic at all - it consumes a value already in hand. A 4.0 is untouched: the DIS read is gated away from it, and the 4.0 serial's source on the wire is not yet identified. Splitting the two is what lets this half ship now instead of waiting on a 4.0 protocol capture. WhoopSerialIdentity is the pure, shared core, and its REFUSALS are the point: adoption migrates every device-scoped row onto the id it returns, so a blank, truncated, or descriptive-prose DIS read must yield nil and leave the strap on its existing id. Moving a history onto a garbage key is worse than not adopting. Serials are upper-cased so one strap read in two cases cannot become two ids, and isAlreadyAdopted is the reconnect early-out: after the first adoption the check is one string compare and touches no database. Both platforms defer the adoption off the BLE callback, mirroring adoptOuraSerial - it re-points the ACTIVE device, and the observers that react tear down the very connection the callback is running inside. On Android it lives in AppViewModel rather than SourceCoordinator, which is documented as inert on the WHOOP path and never touches WhoopBleClient internals; AppViewModel already owns both the registry handle and a scope. The serial is a device identifier, so only its 3-character prefix is ever logged - the rule noteWhoop5VariantFromDIS already applies to the variant line. logSafe enforces it and a test pins that the full serial cannot appear. NOT YET HARDWARE-VALIDATED. This is the BLE path: compiling proves nothing about the serial read, the adoption, or the reconnect. Must not merge until a 5.0/MG night confirms the strap adopts once, that a reconnect is a no-op, and that a re-pair re-anchors onto the existing serial row rather than forking a new one. 8 mirrored test vectors per platform on the pure core. Android 4,091 tests / 0 failures; Kotlin compile, i18n and doc-comment gates clean.
Re-review of my own PR. Adoption fires twice per connect on both platforms - the DIS serial and the hardware-revision characteristic arrive as separate callbacks and each re-runs the variant note - which is safe, because the deferred re-check sees the active id already equals the serial id and returns. That is exactly why the Oura pattern re-checks after deferring, and both twins mirror it. The asymmetry is elsewhere and worth writing down rather than discovering from a capture. Swift adopts INLINE in BLEManager and uses its closure only to notify, so it adopts whether or not anything is listening. Kotlin emits the serial and the adoption lives in the observer, because AppViewModel is what owns the registry handle and a scope while SourceCoordinator is inert on the WHOOP path by design. A connect completing before any observer is wired therefore emits into a null callback on Android and does not adopt. Deferred, not lost - DIS is read on every connect, so the next one with an observer alive adopts. But it means a capture can legitimately show the adoption line on iOS and not on Android for the same strap, and someone comparing the two should know that is timing rather than a bug.
Re-review of my own PR caught a severe defect in it: adoption would have re-pointed the legacy "my-whoop" seed, which is the active id for every existing single-WHOOP install. The consequence is worse than a rename. adoptSerialIdentity migrates every device-scoped row onto whoop-<serial>, but WhoopBleClient.deviceId is documented as never reassigned on the single-WHOOP path - "with one WHOOP it stays my-whoop throughout, byte-for-byte today's behaviour" - so new samples would keep being written under my-whoop while the entire history sat under the serial id. A split history, presenting as data that vanished and then began re-accumulating from zero. The ~47 code paths that still read the "my-whoop" literal directly would all read the empty side of the split. mayAdopt now restricts adoption to a provisional whoop-<uuid> id. That is exactly the multi-strap case this ships for - a strap added through the wizard - and it leaves every legacy install untouched, so this phase is a genuine no-op for the single-WHOOP path it must not disturb. The legacy seed joins the adoption path in #1304, once the literals no longer assume it. That ordering matters and I had it half-stated: my plan said the literal sweep must come after adoption so it is not re-pointed at an unstable identity. The dependency runs both ways - adopting the seed before the sweep breaks the installs the literals serve. Mirrored vectors pin the refusal, that a provisional id is still adoptable, and that another brand's id is never touched. Android 4,092 tests / 0 failures; doc-comment gate clean.
… id (#1303 phase 1) Second instance of the same split-history defect, found re-reviewing the fix for the first one. Excluding the legacy seed protected existing installs, but the provisional path had the identical problem on Swift. adoptSerialIdentity moves every device-scoped row onto whoop-<serial> and DELETES the old registry row. BLEManager's own deviceId - and the copies the Collector and Backfiller stamp rows with at write time - were left on the old provisional id, so the next live sample and the next offload chunk would have been written under an id that no longer exists. Rows orphaned, history split in two, which is exactly what this phase exists to prevent. setActiveDeviceId now runs in the same turn as the adoption, which is the call Kotlin already reaches through SourceCoordinator.pointWhoop (it re-points any non-legacy id, so the adopted serial id qualifies). That asymmetry is why only Swift was exposed. The notification closure was also declared and never consumed - dead API dressed as a seam. It now has a real job: BLEManager holds only the non-observable store, so the store write does not publish, and AppModel mirrors the adoption onto the observable DeviceRegistry. Without it the Devices screen and the source coordinator would keep watching an id that no longer exists until the next relaunch. Android is unaffected: pointWhoop already re-points, and no Kotlin file changes here.
Review of this PR found the one thing hardware validation would have surfaced as data loss. Every strap owns TWO ids: the pairing's own, and a computed sibling keyed `<deviceId>-noop` holding the scored days, detected workouts and metric series the engine derives. adoptSerialIdentity re-keys by exact match on activeId, and `activeId-noop` never equals activeId - so the computed half was left behind while the next scoring pass wrote under `<serialId>-noop`. The old computed history then sits under an id nothing reads again: precisely the orphaned history #1303 exists to prevent, displaced onto the derived side. On a real install that is not hypothetical. The field log from build 384 shows 25 computed day rows under `whoop-FD:..-noop`; adopting the serial would have stranded all of them. Why the "already proven, already shipped" argument did not cover it: the migration IS proven, for the ring. A ring has no computed sibling to strand. The WHOOP path introduces a second id shape the Oura path never had, so reuse of the proven migration is exactly where the gap opened. Both platforms now re-key both id pairs in the same transaction, registry rows touched once for the real pairing only (a computed sibling has no pairedDevice row). Kotlin's 25 explicit re-key calls move into a helper called twice rather than being duplicated. Two tests: the sibling travels, and a PK clash on the computed side resolves canonical-wins like the real id - reachable only after a re-pair that already scored days under the serial's own sibling. Rebased onto current main, which also proves the point about drift: the re-key list had 26 entries when this PR was opened and has 25 now, because #1709 dropped rawImuSample. A hardcoded copy of that list would have silently mis-migrated. Verified: 450 WhoopStore tests and 4862 Android tests, zero failures, doc lint clean. Still DO NOT MERGE pending the hardware night - this fixes a defect in it, it does not validate it.
ryanbr
force-pushed
the
feat/whoop5-serial-identity
branch
from
August 30, 2026 06:48
53f20d6 to
7a61296
Compare
This PR makes a device id BE the strap serial, and the strap log prints device ids everywhere - the Devices list, every dayOwner line, the per-source counts. Neither existing rule catches that shape on either platform: the MAC rule wants MAC form, and the serial rule wants the literal word "WHOOP " followed by a DIGIT, while an adopted id is `whoop-` plus a serial that commonly starts with a letter. Before adoption existed no device id could contain a serial, so this was not a gap. It is one the moment this PR lands, in every log a user shares. The PR already had a privacy test, and it passed, because it covers `logSafe` - the deliberate three-character truncation on the ADOPTION line. Nothing covered the id itself flowing into every other line. A test that pins the narrow surface while the wide one leaks is worse than none, because it reads as coverage. Masking keeps the leading three characters, the same rule logSafe applies, so two straps stay distinguishable in a log. The `-noop` computed-sibling suffix is PRESERVED: it is not identifying, and it is exactly what separates derived rows from measured ones in the Days:/Stored: lines - the distinction today's field diagnosis turned on. Masking it away would have taken a diagnostic with it. The six-character floor matches WhoopSerialIdentity.minSerialLength, so anything too short to be accepted as a serial upstream is not mistaken for one here, and `my-whoop`, `my-whoop-noop` and the already-masked MAC form are untouched. MAC redaction runs first by design: `whoop-<MAC>` is `whoop-FD:••…` by then and cannot be re-mangled. Byte-identical across platforms, verified rather than assumed: StrandTests cannot run on this machine, so the Swift rules were compiled standalone and their output compared against the Kotlin expectations on all five cases - adopted id, computed sibling, legacy seed, MAC form, too-short id. 4866 Android tests green, doc lint clean. The Apple half is app-target, so it needs app-build.
ryanbr
added a commit
that referenced
this pull request
Aug 30, 2026
A testing build cut from the #1455 branch took the staging high-water to 386, and testing-latest with it. Restoring main means clearing that high-water, not re-cutting 385: 385 is a DOWNGRADE against an installed 386, which Android refuses outright with "app not installed" — the #1303-era trap where staging and main counters leapfrog. So versionCode 385 -> 387 and CURRENT_PROJECT_VERSION 266 -> 268, skipping the numbers the PR build consumed. Display version holds at 10.6.1. Contains no PR content: the serial-identity work stays unmerged on its own branch, and its disposable version bump never touched main.
Found on real hardware, which is what the DO NOT MERGE gate was for. A 5/MG adopted its serial correctly: the registry re-pointed, the row migration carried all 25 computed days onto the new id, and the 4.0 was untouched. But NoopApplication.activeDeviceId was a `by lazy` - resolved once at startup and frozen for the life of the process - so every consumer of it kept the PRE-adoption id. The diagnostics export printed the old address-based id while the Devices list showed the serial id, and, worse, the scoring pass went on deriving days under the old id: the computed history split across BOTH ids and only healed on the next cold start. The field logs show the whole arc. Immediately after adoption the export read `Active deviceId: whoop-FD:..` with `..-noop=21` and no serial-id bucket at all; after a full phone restart the same install read `Active deviceId: whoop-MGB..` with `whoop-MGB..-noop=25`. Nothing was lost, but for the life of that process the app wrote to an id the registry no longer considered active. The handle now resolves lazily on first read and follows [onActiveDeviceAdopted], which adoption calls alongside setActive and the coordinator notify. Only the handle moves - the registry write and the migration already happened, and the BLE client is re-pointed by its own caller - so it stays narrow and idempotent. The invariant is pinned against source, since NoopApplication needs a Context and cannot be built in a JVM test: activeDeviceId must not become a `by lazy` again, the hook must exist, and adoption must actually call it. All three would have been green before this change, which is the point. 4867 tests, doc lint clean. Adoption itself is now hardware-validated on a 5/MG; this was the last defect that validation surfaced.
This was referenced Aug 30, 2026
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
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.
DO NOT MERGE until hardware-validated — see below. Opening it now so the design can be reviewed
while a 5.0/MG night is arranged.
Phase 1 of #1303, the multi-strap keystone.
Why this is smaller than it looks
WHOOP identity is a transient CoreBluetooth UUID / Bluetooth address, so a re-pair or factory reset
mints a fresh one and the same physical strap forks into a second registry row, orphaning its history
(#1193).
The scary part — re-keying every stored row — is already written, shipped and proven:
adoptSerialIdentitymigrates everydeviceScopedTablesrow, is PK-clash-safe, is unit-tested, and hasbeen running on both platforms for the ring since #771. This PR does not add a migration. It calls the
proven one from the WHOOP path.
Why 5.0/MG only
Their serial is already read at connect from DIS
0x2A25for MG-vs-5.0 discrimination — so this addsno BLE traffic whatsoever, it consumes a value already in hand.
A 4.0 is untouched: the DIS read is explicitly gated
!= .whoop4, and the 4.0 serial's source on the wireis not yet identified (
REPORT_VERSION_INFO's 35 bytes are fully accounted for as firmware). Splittingthe two is precisely what lets this half ship now rather than waiting on a 4.0 protocol capture — that is
Phase 0/2, and it needs a capture, not speculative code.
The refusals are the feature
Adoption migrates every device-scoped row onto whatever id is returned, so
WhoopSerialIdentityrefuses ablank, truncated, or descriptive-prose DIS read and returns nil, leaving the strap on its existing id.
Moving a history onto a garbage key is worse than not adopting. Serials are upper-cased so one strap
read in two cases cannot become two ids, and
isAlreadyAdoptedis the reconnect early-out — after thefirst adoption it is one string compare and no database work.
Design notes worth reviewing
adoptOuraSerial: adoption re-points the ACTIVEdevice, and the observers that react tear down the very connection the callback is running inside.
AppViewModel, notSourceCoordinator— that file is documented as inert onthe WHOOP path and never touches
WhoopBleClientinternals.AppViewModelalready owns the registryhandle and a scope. (I wired it into SourceCoordinator first and reverted; the file's own header says why.)
noteWhoop5VariantFromDISalready applies.logSafeenforces it and a test pins that the full serialcannot appear in a shareable strap log.
Verification
cases)
What must happen before merge
This is the BLE path — compiling proves nothing. A 5.0/MG night must confirm:
scenario