CI: frame integrity verdict (upstream PR ryanbr/noop#1985) — macOS/iOS app build and StrandTests - #105
Closed
bhelm wants to merge 333 commits into
Closed
CI: frame integrity verdict (upstream PR ryanbr/noop#1985) — macOS/iOS app build and StrandTests#105bhelm wants to merge 333 commits into
bhelm wants to merge 333 commits into
Conversation
Field log from build 384 read "Deferred once so far — expected on the connect that asks" for a strap that has never completed a handshake. The count was per app process, and the app had restarted; a log is usually exported well AFTER the restart that hid the run. So the one line meant to distinguish "the experiment is working" from "this has never once worked" was stating the former about the latter. The run moves to SharedPreferences. Reads are guarded and fall back to 0, which under-states rather than inventing a history. Cleared exactly where the run genuinely ends - a hello the stack accepted, and a genuine bond - and NOT in reset(), which runs on every disconnect and would defeat the point. The guidance latch stays per-process on purpose: a new process should state the full paragraph once more, because whoever just restarted the app is the reader most likely to need it. Two things this caught on the way through. The rename broke the ordering contract test that pins "the run is only cleared AFTER the stack accepts the write" - it had pinned the old spelling, so it failed loudly rather than silently passing over a moved invariant, which is what a source contract is for. And the old field's doc block was left stacked above the new one, still claiming "per app process, like the override budget" - true when written, false the moment the value was persisted, and exactly the stale-comment shape this file has been cleaning up all week. Removed rather than reworded, since the new block says it. 4852 tests green, doc lint clean, no new warnings.
ryanbr#1738 gave the Android live-persist catches a voice and said plainly that the Swift twins at Collector.swift:218 and :274 were NOT fixed. They still swallowed the error while re-buffering, so a store rejecting every insert produced a log full of `rr emit ... offered=N` and no sign that none of it landed - instrumentation that reads like success, which is the worst shape a diagnostic gap can take. Both paths report now, each naming its transport, rate-limited to a minute, with a consecutive-failure count. Per-transport because the standard 0x2A37 path and the puffin REALTIME_DATA path (ryanbr#1118) fail independently: a shared counter would let one path's success reset the other's run and report a persistent failure as a string of first-failures. @mainactor isolation makes the counters safe without a lock, unlike the Kotlin twin's AtomicInteger, whose two flushes can genuinely run concurrently - the asymmetry is deliberate and recorded at the site. The line builder lives in StrandAnalytics rather than beside its caller, because Collector is app-target Swift with no default CI, and the package is where the other emitted-line builders already sit and get tested. Byte-identical to the Kotlin rendering, and now provably so. Both suites assert the SAME whole-line literal rather than `contains` fragments - every fragment check would still pass with a stray space or a moved clause, so equality is the assertion that actually holds the two platforms together. The Kotlin side gained that assertion here too; it had only fragments before. Verified on Linux: 1656 StrandAnalytics tests and 4853 Android tests, zero failures. Collector itself is app-target, so its type-check needs app-build.
…wins versionCode 384 -> 385, CURRENT_PROJECT_VERSION 265 -> 266. Display version holds at 10.6.1: v10.6.0 is still the latest release, so only the build numbers move. 385 is +1 on the staging high-water, so a sideload updates in place. Carries six commits over 384. The one that changes what a strap log SAYS is the persisted hello-deferral run: 384 reported "Deferred once so far" for a strap that has never completed a handshake, because the count reset with the process. The rest are the Swift live-persist twin, the Linux package work, and the Kotlin warning cleanup - none of which alter behaviour on the phone.
A testing build cut from the ryanbr#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 ryanbr#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.
…1455) WHOOP identity is otherwise a TRANSIENT CoreBluetooth UUID or 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 (ryanbr#1193). The 5/MG already reads its own serial from DIS 0x2A25 at connect for MG-vs-5.0 discrimination, so this consumes a value already in hand and adds no BLE traffic. A 4.0 is untouched: it exposes no DIS serial and the 4.0 serial's source on the wire is still unidentified, so there is nothing honest to adopt onto. The refusals are the feature. Adoption migrates every device-scoped row onto whatever id is returned, so a blank, truncated or descriptive-prose read yields nil and leaves the strap where it is - moving a history onto a garbage key is worse than not adopting. Serials are upper-cased so one strap read twice cannot become two ids, and the legacy `my-whoop` seed is deliberately EXCLUDED from adoption until ryanbr#1304 retires the ~47 literals that still assume it. THREE DEFECTS WERE FOUND BEFORE THIS LANDED, and the last one is why the hardware gate existed. Review found that the migration re-keyed by exact match on the pairing id, which never equals its COMPUTED sibling `<deviceId>-noop` - the scored days, detected workouts and metric series the engine derives. That half stayed behind while the next scoring pass wrote under the new sibling, stranding the computed history under an id nothing reads again: the orphaned history this exists to prevent, displaced onto the derived side. The "already proven, already shipped" reasoning did not cover it because a RING has no computed sibling to strand. Review also found that once a device id IS the serial, the strap log prints it everywhere - the Devices list, every dayOwner line, the per-source counts - and neither redaction rule caught that shape. The PR's own privacy test passed throughout, because it covered the deliberate three-character truncation on the adoption line and not the id flowing into every other line. HARDWARE found the third. On a real 5/MG the registry re-pointed and all 25 computed days migrated correctly, but `NoopApplication.activeDeviceId` was a `by lazy`, frozen for the life of the process, so the scoring pass went on deriving days under the PRE-adoption id and the computed history split across both until the phone was restarted. No code reading could have surfaced that; it needed a strap adopting a real serial. VALIDATED: one Android 5/MG, fresh-adoption path, one serial format. Adoption fired two seconds after the DIS read on an UNBONDED link, all 25 computed days arrived under the new id, the paired 4.0 was untouched, and the redaction held - the serial appears nowhere in the resulting logs. ACCEPTED, KNOWINGLY: the Apple path is NOT hardware-validated. It defers off the CoreBluetooth callback with different timing, uses GRDB, and re-keys every device-scoped row, and neither platform is behind a feature flag - so this reaches iOS and macOS users on their next connect. The re-pair merge branch and other serial formats are unit-tested only. What bounds the downside is the refusal path: an unexpected serial yields nil and the strap keeps its existing id, so the failure mode for an unfamiliar shape is no adoption rather than corrupted ids.
Field regression, dated. ryanbr#1646 landed the opt-in pairing experiment on 2026-08-26; a 5/MG stopped syncing that same day and had not synced since. The mechanism is the one ExplicitBond already documents: a strap answering SMP 0x05 defers its CLIENT_HELLO to a "next connect" that requests a bond and defers again. No hello means no bond, no bond means no SET_CLOCK, and an un-clocked 5/MG does not bank to flash at all - so there is nothing to offload even while the link is healthy. Live HR streamed the whole time, which is what made it look fine. Deferring ONCE is the honest form of the original reasoning: give the pairing the connect it asked for. Deferring again is not caution, it is repeating an experiment whose answer is already in. The threshold is settled by what each error costs. One deferral too many costs a user their entire history sync, silently and permanently - four days here, thirteen backfill deferrals in one session, not one hello written. One hello too early costs at worst a link drop and reconnect, which the bond watchdog already handles. The escape hatch was a SECOND opt-in switch, so avoiding the outage required discovering and enabling the hello override as well. Nobody would infer that. Two pieces of copy did real damage and are fixed here too. The live Test Centre description said the switch "does not require" - true, and silent about the consequence. It now says a refusing strap defers its handshake while this is on. (The Settings copy of this card is dead code behind `if (false && ...)`; the live control moved to Test Centre, and I nearly fixed the dead one.) The deferral line I added today printed "(override off)" one clause after the words "the pairing experiment", and it was read - correctly, on the plain text - as the EXPERIMENT being off. It was on the whole time. A parenthetical reporting one setting's state while the sentence names another has to say which is which; it now names both. 4871 tests, i18n and doc lint clean. BLE behaviour change: needs a strap to confirm the hello actually goes out on the second connect.
versionCode 387 -> 388, CURRENT_PROJECT_VERSION 268 -> 269. Display version holds at 10.6.1. 388 is +1 on the staging high-water, so it installs over both the main-line 387 and the PR-branch 386 without Android refusing a downgrade. The change to test on hardware: with the pairing experiment ON, the CLIENT_HELLO is deferred for ONE connect instead of forever. On an install whose deferral run is already non-zero the hello should go out on the FIRST connect after installing. The line "writing CLIENT_HELLO" has been absent from every field log so far; that line appearing is the result to look for.
Field-caught on build 388, in the log that proved the deferral fix worked. The hello finally went out at 21:00:47 - first time in the whole ryanbr#1635 thread. It was never acked, the bond watchdog bounced the link seven seconds later, and the reconnect deferred again: the run had been reset the moment the hello was WRITTEN, so the strap alternated defer, write, bounce, defer forever. That is the five-second loop ryanbr#1642 removed, reintroduced through a different door. Clearing the run on the write encoded "we sent one, so the deferral is over". The run means attempts, and only a genuine bond ends it. With the reset gone, the deferral costs one connect as intended and the ryanbr#1635 suppression latch bounds the attempts after that, settling the link into the "Live HR, not fully paired" state the app already models - the designed end state for a strap that answers SMP 0x05, rather than an indefinite bounce. The contract test that guarded this now pins the OPPOSITE invariant: writeClientHello must not clear the run at all. It previously asserted the clear happened after the stack accepted, which was the right question one revision ago and the wrong one now. Worth recording what this log settled beyond the fix. The strap has never bonded - seventeen unacked hellos across the thread - and "Last sync: 4d ago" was never its own: KEY_LAST_SYNC_AT is a single global key, and that timestamp belonged to the 4.0, which had not been worn for three days. The MG's own buckets read zero rows throughout, before any change today. There was no sync regression to find on this strap; there was a strap that has never offloaded, and a global label that read like it had. 4871 tests, doc lint clean.
ryanbr#1742) The 5/MG historical offload has never been attempted on a strap that does not bond, and the blocker turns out to be ours rather than the strap's: beginBackfill refuses while connectHandshakeDone is false, and for a 5/MG that flag is set in exactly one place, behind the CLIENT_HELLO ack. On a strap answering SMP "Pairing Not Supported" that ack can never arrive. requestSync's own bonded check already passes on these links, so that single line is the whole gate. The link underneath is not the broken thing it was taken for - it holds for tens of minutes with the hello suppressed, live HR streams throughout, the unbonded DIS read succeeds, and puffin commands go out without the stack objecting. What is genuinely unknown is narrower: those writes go out WITHOUT response, so their GATT_SUCCESS is Android describing its own controller, and the offload arrives on notify chars this app has never once subscribed on a healthy unbonded link. The belief that it cannot work is a comment carried from iOS, not a measurement taken here. So this asks in stages, each falsifying the next: subscribe the puffin notify chars (an insufficient-authentication status is the whole answer), then a read-only GET_CLOCK (a COMMAND_RESPONSE is the first hard evidence the strap acts on puffin commands unbonded), then the clock and the ordinary offload. Default off, its own switch, a refusal latched per device and silence bounded by a small per-process budget. It never sets didBond - that flag is the record of an acked hello and every reader of it is reasoning about a handshake, so setting it to unlock the offload is the false-bond bug arrived at deliberately. Its own writes go out without response, because a with-response write to fd4b0002 is exactly what the hello does and what leaves the write queue starved behind a callback that never comes. Four review passes. The last one caught the one that mattered: the 8s verdict timer was an anonymous lambda on a link that reconnects in about 3, so an old link's timer could conclude a new link's probe early with a verdict it never waited for and spend the silence budget on it. Three of those retire the experiment - the instrumentation would have answered ryanbr#1635 the wrong way by itself. Named runnables cancelled in reset() now, the ryanbr#314 pattern this file already uses. Not hardware-validated, which is the entire point of it. Android only, matching the explicit-bond experiment. A Swift twin is worth writing if and only if Android's answer is yes. 4889 tests, lintVital, doc-comment lint and the i18n gate clean. Six locales.
Build numbers only - 10.6.1 stays the unreleased line. 389 clears the staging high-water so the APK installs over 388 rather than being refused. Carries the ryanbr#1635 unbonded offload probe, which is DEFAULT OFF: nothing changes for anyone who does not turn it on under Settings -> Experimental ("Try history sync without pairing"), or Test Centre -> 5/MG protocol diagnostics. What to expect once it is on, in the strap log, on a link with no CLIENT_HELLO: "Unbonded offload probe: subscribing the puffin notify chars" - stage 1 started. Then one of: - "subscribe of fd4b0003... failed GATT_INSUFFICIENT_AUTHENTICATION" - the whole answer. The offload needs an encrypted link and a strap refusing SMP can never sync history. Latched, so it says this once. - "N of 4 puffin notify chars subscribed... Sending GET_CLOCK" - stage 2. Then either "COMMAND_RESPONSE received", which clocks the strap and requests the offload, or "no COMMAND_RESPONSE after 8000ms", which spends one of three silence attempts. If it does reach the offload, expect the log to say so up front: this strap has never been clocked by NOOP, and an un-clocked 5/MG does not save sensor data to flash, so GET_DATA_RANGE may report little or nothing banked. That would mean history works from now on, not that the probe failed. Live HR is untouched either way. The probe's own writes go out without response precisely so a strap that ignores them cannot wedge the write queue the way the CLIENT_HELLO does.
…r runs (ryanbr#1743) Follow-up to ryanbr#1742, caught on the first field capture taken with any of it in mind: the probe as merged would essentially never have run. shouldProbeUnbondedOffload requires a link carrying no CLIENT_HELLO, and that gate is right - a subscribe refusal on a link the bond watchdog is about to bounce is unattributable, which is the ambiguity this thread has been paying for since June. But the probe is only SCHEDULED from the two no-hello branches, and on this strap neither is reached. Across 41 captures the suppression latch fired in three, all on 26 Aug, and never since; the explicit-bond deferral yields after its first connect by design (ryanbr#1642), so every link after that writes a hello. The 22:21 capture shows the pull-to-refresh reaching the offload and being turned away by exactly this gate twice in three seconds, with helloWrittenThisLink=true both times. The resolution is not to loosen attributability. It is that these are mutually exclusive experiments: "does the hello work" and "is the hello needed at all" cannot both be asked of one link, because asking the first is what destroys the second's link. Opting in now supersedes the handshake for that connect - the explicit pairing request as well as the hello - placed ahead of both, since the deferral branch returns early and a probe scheduled after it never runs. Pressing Connect still gets the handshake, exactly as it overrides the suppression latch. An explicit request for one experiment must not be answered with another. The line says the hello is absent BY CHOICE, because an absent hello reads identically to one that failed silently, and it names the explicit-bond switch when that is also on - a pairing in flight makes a refusal unattributable, so the probe declines to latch it and the capture is weaker for nothing. Also puts versionCode in the strap-log header. Build identity has been ambiguous three times in two days; dating the 22:21 capture depended on noticing that probe lines were absent. Staging 390 / iOS 271. 4893 tests, lintVital, doc lint and i18n clean.
…yanbr#1744) The second false bond, from the 30 Aug 22:47 capture. Unlike the first this is not a mis-attributed completion - the completion really is the hello's. The error is what was concluded from it. 22:46:31 CLIENT_HELLO outcome: acked by fd4b0002 after 5ms status=GATT_SUCCESS(0) 22:46:31 [connection] bondState encryptedBond family=whoop5 (CLIENT_HELLO acked) 22:46:33 bond state poll: BOND_NONE One link, two seconds. The app showed "Bonded, streaming." for it, on a strap an HCI capture already shows answering SMP "Pairing Not Supported" - where an encrypted bond is not merely absent but impossible. A write completion says the STACK finished the write. Encryption is a property of the LINK, and on Android only the OS bond state attests it. Those are now separate: encryptedBond waits for the OS bond state or the strap's own BLE_BONDED event, while the completion still drives the HANDSHAKE - subscribe the puffin chars, clock the strap, offload - all of which need the strap to be listening, not the link encrypted. That split keeps the only path that has ever plausibly led to history on this strap and drops the claim that was false. Everything else in the branch asserting the strap's WILLINGNESS to bond is gated on real encryption too: noteGenuineBond, clearPairingHint, staleDirectFailures, the deferral run and the override budget. Clearing those on an unencrypted completion is how a strap that has never bonded gets recorded as one that bonds fine - and it retires the very counters that exist to notice it never did, so the more it fires the less evidence survives that it fired. Timing is diagnostic, never a gate. Across 41 captures every hello either "completed" in 0, 4, 5 or 7ms or produced no callback at all about 3150ms later; nothing in between, and 0ms cannot be a round trip. The line says so when the elapsed is under one connection interval. It decides nothing - a peripheral may answer inside the same connection event, so a strap that genuinely bonds must never lose its bond for being quick. Android-only on purpose: CoreBluetooth exposes no bond or link-encryption state, so a shared helper would be handed a constant false and would suppress genuine bonds there. Staging 391 / iOS 272. 4898 tests, lintVital, doc lint and i18n clean.
Reported three times before it was believed, which is the part worth keeping: the number was plausible, so the reading was treated as mistaken rather than the label. `noop.lastSyncAtSec` is ONE key, stamped on any strap's HISTORY_COMPLETE and read back for whichever strap is active. On a single-strap install those are the same strap, which is why it survived this long. With two paired it reports the other one's - the capture shows "Last sync: 4d ago" beside "Days: whoop-MGB...=0" and a 4.0 last seen three days earlier. The 5/MG had never banked a row. The timestamp on its screen was the 4.0's. Wrong in the reassuring direction, which is what made it expensive: a strap that had never synced read as recently synced, so a pull-to-refresh that could not possibly change the number looked broken rather than inapplicable - and it sent an entire investigation after a sync regression that never existed. Same defect resolveFirmware fixed for firmware strings, so it takes the same shape: stamped per BLE address, read per strap, and the legacy global honoured ONLY when exactly one strap is paired - the one case where it cannot be ambiguous, which keeps single-strap installs reading correctly across the upgrade. Deliberately NOT migrated by writing the global onto the active strap: that is the same guess in a different place, and on a multi-strap install it would bake the wrong attribution in permanently instead of self-correcting at the next real sync. The #57 write-health pair had it too, one line below in the same capture: "Data write: rows last landed 4d ago" against a strap whose own row count was zero. Both halves are scoped, not just the noticed one - they are read as a pair ("stalled more recently than ok" is the alarm), so scoping one would compare a strap's own stall against another strap's success. The second commit is the same bug surviving inside the fix for it, caught by a question about what the screen would actually show. The constructor seeded from NoopPrefs.lastDevice, which records the last strap to BOND - and `bonded` is set for a 5/MG by the live-HR path, so on a two-strap install that is whichever strap was worn most recently, not the one the screens are scoped to. Wear the 4.0 for an evening and its sync time would appear on the 5/MG's Today screen at the next launch. Seeded from the registry's ACTIVE row now; that read is suspend, so the constructor seeds nothing and the value arrives a moment later. A blank moment is honest, and there is no cheap synchronous guess that is not the bug in miniature. For the strap in the capture the answer is now "never (this strap)". Not a degraded answer - the correct one, and the one nobody had been given. No Swift twin needed: the Apple side has no persisted global equivalent. Staging 392 / iOS 273. 4907 tests, lintVital, doc lint and i18n clean.
"Refresh doesn't work" - reported four times. It worked. It was silent.
Every manual-sync control gated on `bonded`, and `bonded` is not the condition: the
live-HR path sets it for a 5/MG that has never completed a handshake, so the control
was offered, accepted, and then declined by beginBackfill's own connectHandshakeDone
gate with nothing shown anywhere. The field log has the pull landing twice in three
seconds and being turned away both times, into the log and nowhere else.
LiveState now carries `historyReady`, published at exactly the three sites that set
connectHandshakeDone and cleared at the one that clears it, so the two cannot drift.
The Today gesture, the Live "Sync now" button and the Health sync card gate on it.
This cannot withhold a sync that would have run: connectHandshakeDone is already a
precondition of beginBackfill, so the controls disappear exactly where the offload
would have been refused regardless. reset() runs on connectToDevice as well as
handleDisconnect, so a stale true is cleared before any new link sets connected, and
canRequestSync's own `connected` check covers the gap either way. On a WHOOP 4.0 that
has bonded nothing changes at all - both flags are set in the same callback eleven
lines apart - and a 5/MG that completes its handshake keeps every control it had. It
also catches a case a strap-family check could not: a 4.0 whose bond has not landed
has an equally dead sync button, and that goes away too.
The re-review is what earned the second commit. Gating the button alone made the
Health card contradict itself on exactly the strap the gate is for: title "Ready to
sync", not pulsing, and a helper line promising an immediate sync, all sitting above
a DISABLED button. Before the gate the three agreed - wrongly, but consistently - and
gating one of them is what split them.
The helper line turned out to be the prize. Its `!bonded` branch already said the
right thing ("Finishing the pairing handshake. Sync now becomes available once the
strap is paired.") and simply never fired on the strap that needed it. So the
explanation this gate was missing was already written and translated; it needed the
correct flag, not a new string in six locales.
Swept every live.bonded in the UI while checking. The rest are correctly about the
LINK rather than history - haptics, buzz, alarm arming, "Live HR (not fully paired)",
onboarding - all of which work on an unbonded 5/MG and must keep working. None moved.
NOT gated: the three syncNow() calls in Settings and Test Centre. Those are
re-analysis nudges after an HRV-window or baseline change, not sync controls; gating
them would stop a re-score for a reason unrelated to re-scoring.
Staging 393 / iOS 274. 4909 tests, lintVital, doc lint and i18n clean.
…#1749) First field run of the unbonded offload probe. It got its answer and exposed two defects in itself doing it. The capture: 16 probe starts, ZERO verdicts of any kind, zero confirmed subscribes, zero refusals, every link dying 10.8s into the connect - about three seconds after the CCCD writes went out. Stage 1 ended with the LINK, an exit the probe had no branch for, so it concluded nothing, the silence budget never advanced, and it re-ran on every reconnect indefinitely. That is precisely the unbounded retry shouldProbeUnbondedOffload's own doc claims the design prevents, reintroduced through an unhandled exit - and it cost a strap bouncing every 17 seconds all night. That exit is now a verdict, charged to the silence budget, because "the link will not survive being asked" is a stronger reason to stop asking than a strap that merely stayed quiet. It is also a FINDING and the line says so. No callback AND no ATT error, then a teardown about three seconds later, is the CLIENT_HELLO's own signature on the same service. It does not prove the puffin characteristics require encryption, but it is what that would look like from here, and it says plainly that the offload is not reachable on this strap without a bond. The probe was built to answer that question either way, and this is the answer. Second defect, in the line the capture is read through: the supersede message warned that "Ask Android to pair" being on makes a refusal unattributable - while the very same branch returns BEFORE the pairing request, so no SMP can be in flight. The capture confirms it: not one probe connect carries a createBond. A caveat that could not apply, printed on every probe connect, and it briefly cast doubt on a clean result while reading the log. The re-review then found the identical hole one stage lower: a link lost during the GET_CLOCK wait also reported nothing and spent no attempt. Both stages report now, with DIFFERENT lines - stage 1's loss carries the signature above, stage 2's carries no finding at all, since the subscribes had landed and the strap was still inside its window. Conflating them would manufacture evidence from an inconclusive link. The test pinning the old supersede wording asserted "unattributable" and had to be repinned - the same shape as the writeClientHello contract test earlier in this thread. A test can be correct and then quietly become a guard on the wrong behaviour. Staging 394 / iOS 275. 4913 tests, lintVital, doc lint and i18n clean.
…what it can do (ryanbr#1753) Both defects Android fixed today were already present here, in the same shapes, and one of them sat eighty lines from its own fix. LAST SYNC. `lastSyncedAt` is ONE defaults key, stamped on any strap's HISTORY_COMPLETE and read back for whichever strap is active. Single-strap installs never notice; with two paired it reports the other one's. DebugDataDiagnostics already called FirmwareAttribution.resolve for firmware and then read last-sync globally eighty lines above it - the pattern was in the file, last-sync just never got it. LastSyncAttribution mirrors FirmwareAttribution one for one: per-BLE-identifier keys, and the legacy global honoured ONLY when exactly one strap is paired, the one case it cannot be ambiguous. The #57 write-health pair is scoped too, both halves, because they are read together ("stalled more recently than ok" is the alarm) and scoping one would compare a strap's own stall against another's success. Both initialisers now seed NOTHING. The obvious cheap seed is the global key, which belongs to whichever strap synced last - on a two-strap install not the one the screens are scoped to. seedLastSyncFromActiveStrap resolves it from the registry's ACTIVE row at bootstrapStore, the first point the registry exists. HISTORY READY. Every manual-sync control gated on `bonded`, which the live-HR path sets for a 5/MG that has never completed a handshake - so the control was offered, accepted, then declined by beginBackfill's connectHandshakeDone guard in silence. HealthView's own comment conceded the gate it was matching. This cannot withhold a sync that would have run: connectHandshakeDone is already that precondition. The card's pill, pulsing and helper line move WITH the button, or gating one of the three makes them contradict - "Ready to sync" above a disabled button. Three review passes, each finding something. One: Today's gate was on the pull's RELEASE, which would have left refreshArmed stuck true for the rest of the gesture since that branch is the only thing clearing it - a worse failure than the silent one being fixed. It gates the ARM now, which also withholds the haptic, reading honestly as unavailable rather than unresponsive. Two: moving the stamps to per-device keys removed their only writers and left three readers behind. The prefs-only debug header would not have become per-device, it would have frozen at its pre-upgrade value and then read "never" and "no rows ever persisted" forever, for everyone. The firmware precedent had this right and it was missed: FrameRouter still writes noop.lastFirmware globally beside ryanbr#1634's per-device write. Both stamps dual-write now; every reader that CAN attribute still prefers the per-device key. Three: LastSyncAttribution shipped without tests while its Kotlin twin had a full set, next to an existing FirmwareAttributionTests. Parity is not just the helper. NOT ported: the unbonded offload probe, whose answer on the strap that motivated it is no - a twin would port a dead end. The write-completion-is-not-encryption split has no twin either: CoreBluetooth exposes no bond state, so the Apple fix needs the strap's own BLE_BONDED event instead. app-build green on both legs, and its Strand leg RUNS StrandTests. No new copy, so no xcstrings churn. NOT hardware-validated on Apple.
Replaces the flat 14-night sum with a bounded, repayable next-night target on both platforms: currentNeed = personalizedBaseNeed + currentDebt nextDebt = 0.55 x max(0, currentNeed - creditedSleep) The old ledger gave all fourteen nights full weight, so a repeated shortfall grew into balances like eleven hours that cannot answer the only question the number is for: how much should I add to tonight's target. The recurrence is self-limiting because yesterday's debt is already part of today's need - meet the full current need and the displayed debt clears - and a calculated debt under ten minutes reads as balanced. Verified rather than taken on trust. The recurrence was reimplemented independently and run against both suites' vectors, which agree: [360, 546, 540] clears, and the 9.9 / 10.0 deadband boundary lands either side as documented. Fifteen tests on each platform, one-to-one by name. Every caption and colour boundary on both sides goes through the named constant - no hardcoded tens anywhere in the touched UI. The full Android suite passes on the branch, and all fifteen CI checks are green. The rounding parity is the detail worth calling out: Swift rounds -debt and Kotlin negates round1(debt), which agree only because both are half-away-from-zero - and Kotlin's is hand-written with a sign branch precisely because Math.round is half-UP. That is the class of divergence parity work usually ships without noticing. The 0.55 coefficient is calibrated against one contributor's 853 consecutive exported WHOOP nights (93.0% within 15 minutes, 94.5% within 20), and the PR says so plainly - including that the sleep-restriction literature it cites does NOT derive the constant. Single-user interoperability approximation, one constant, and trivially retuned later. Also collapses the competing UI debt calculations into one series per platform, so card, trend, night detail and Android's tapped detail can no longer disagree. One thing for release notes rather than review: this materially changes a user-visible number. Someone carrying an eleven-hour balance will open the app to something under an hour. Correct, and the whole point, but it wants a What's New line or it reads as data loss. Thanks @bhelm - the test parity, the constant discipline and the honesty about what the coefficient is and is not are all well above the bar.
…G ack (ryanbr#1746) GET_DATA_RANGE answers twice - a short PENDING(2) acknowledgement, then the payload with SUCCESS(1) - and the ryanbr#689 backlog decode ran on both. The ack carries no ring pointers by construction, so it decoded to null and logged "offsets may have moved" once per sync on healthy hardware, pointing every reader at an alignment bug that was not there. The tree already knew: Framing's own result-code table records "2=PENDING precedes SUCCESS on GET_DATA_RANGE (hardware-confirmed, #78 fork)". The decode site simply had no way to act on it, because a too-short frame and a misaligned one look the same to pagesBehind. A pure isPendingResponse(frame, cmdOff) on both platforms reads the result byte at cmdOff+2 and gates the decode at the two call sites. The SUCCESS frame that will not parse still logs exactly as loudly as before - that is the case ryanbr#818's comment protects, and it is untouched. The ryanbr#451 raw dump stays unconditional above it. Verified against the captured bytes rather than the description. On the PENDING frame, index 10 is 0x22 (34, GET_DATA_RANGE), 11 is the origin sequence, 12 is 0x02 - the cmdOff+2 claim is exact. That fixture is a whole frame: declLen 12, +8, and it is 20 bytes. The SUCCESS fixture is trimmed from its declared 84 bytes to 40, which is deliberate rather than damaged: pagesBehind reads to index 37, so the trim keeps precisely what the decoder needs, and decoding it by hand gives w=48833, u=48820, t=131072 -> 13, matching the log excerpt in the PR. The two predicates are exact twins, including the detail that Kotlin masks the signed Byte where Swift's UInt8 does not need it. Five new cases per platform, one-to-one. Merged onto a main that had moved a long way under it - WhoopBleClient.kt was rewritten substantially by ryanbr#1744, ryanbr#1748 and ryanbr#1749 after this branch was cut - so it was merged locally and run rather than trusted: 4,926 tests, 0 failures, DataRangeScanTest 17/17, doc lint and i18n clean, all fifteen CI checks green. One thing left on the table for later, since the fixture already supports it: nothing asserts pagesBehind == 13 on that SUCCESS frame. The pair currently pins only that the two are distinguishable; asserting the value would pin that the pair behaves exactly as the log shows, for free, on both platforms. The PR body was edited before merge to remove a closing verb adjacent to an issue number. It was a denial - the contributor was explicitly saying the issue should stay open - but GitHub matches those keywords literally and does not read negations, so it would have been actioned on merge. Harmless this time only because that issue was already closed. Thanks @Zebsi235 - real captured frames rather than synthetic ones, the failing stub run to prove the tests actually catch the regression, and the Swift gap stated up front rather than left to be found.
…s on a clean tree on Windows (ryanbr#1747) Every baseline in Tools/ stores forward slashes, and six sites keyed their lookups on str(path.relative_to(ROOT)) - OS-native, so on Windows every lookup missed. On a clean checkout that reported 236 + 63 NEW literals and eleven failing locale allowances, while the same run printed STALE for the very entries it was failing against. Two messages pointing opposite ways, and the natural conclusion - the baselines are stale, regenerate them - would have wiped the ratchet. CI runs on ubuntu, so only a contributor running the repo's own gate locally on Windows ever saw it, and then went hunting 299 regressions they did not cause. Verified rather than taken on description. The baseline really is forward-slashed (sampled: android/app/src/main/java/com/noop/ui/AppRoot.kt). The change really is a no-op on POSIX: the audit run on main and on this branch in place produces byte-identical output, exit 0 both, and the tool's own suite passes. The six converted sites are exactly the behaviour-affecting ones. All thirteen relative_to(ROOT) sites were classified: the six changed build lookup or dict keys, and the seven untouched are pure print() - including 1227's `line`, which only feeds the human-readable summary. A principled split, not a partial job. Those seven will still render backslashes on Windows, which is cosmetic and worth folding in later. Worth doing next, and it is the more valuable half: this is the SECOND tool with this exact bug (doc_comment_lint was ryanbr#1691), and the fix is six inlined as_posix() calls. Six sites is six chances for a seventh to be added wrong, which is how it happened twice. A one-line helper - rel(p) returning p.relative_to(ROOT).as_posix() - collapses them to one, makes the rule impossible for the next reader to miss, and makes it TESTABLE, which it is not today because the conversion is inlined at every call site. Neither this PR nor ryanbr#1691 carries a regression test, and there is currently nothing to assert against. Merged as-is because the fix is correct and a Windows contributor is blocked by it today; the helper is a follow-up rather than a condition. Thanks @Zebsi235 - the before/after on a clean tree, and stating plainly that Linux and macOS were reasoned about rather than run, are both exactly right.
Adds `noop-local-access query <tool> [--flags]` for the five existing local read
tools, and routes MCP and the CLI through one read-only dispatcher so the two can
no longer drift.
The refactor is faithful. The 55 lines leaving MCPServer move verbatim into
NoopToolDispatcher - the switch bodies were diffed statement by statement. The one
behavioural change is the resource path, which used to call
`data().healthSnapshot(days: 14)` directly and now goes through
`dispatch(name:arguments:)`; that routes 14 through
`boundedDays(.int(14), default: 14, max: 120)` = min(max(14,1),120) = 14. Inert.
Run rather than reasoned about. The contributor could not execute Swift locally and
said so plainly, so this was built and exercised here with the Linux
snapshot-SQLite setup from docs/BUILD.md - 14 tests pass, matching the reported CI,
and then the binary itself:
query bogus_tool -> exit 64, "unknown query tool"
query metric_series -> exit 64, "metric_series requires --key"
query health_snapshot --nope 5 -> exit 64, "unknown query flag"
query data_freshness --days 5 -> exit 64, "--days is not supported for data_freshness"
query ... --db-path /tmp/SECRET-PATH-abc/whoop.sqlite
-> exit 1, "NOOP database is unavailable"
That last one is the one that mattered: LocalAccessError.databaseUnavailable carries
the path inside its message, and main.swift replaces it with a constant, so the path
does not reach stderr. Usage errors do not echo unvalidated input either, and stdout
stays empty on every error path, so a caller piping JSON never receives a malformed
value. `codex-config` and `mcp` both still work after the refactor - tools/list
returns a valid JSON-RPC result.
The parser is properly allow-listed: tool names, flags, per-tool flag validity,
duplicate detection, no trailing positionals, required-value checks. `--days -5`
clamps to 1 rather than doing anything surprising. And the strongest test is the
right one - testAllFiveQueriesDispatchThroughTheSamePayloadAsMCP asserts CLI output
equals MCP output for all five tools, which is exactly the guard this refactor needs.
Two things noted rather than changed. Sanitisation is now asymmetric: the CLI hides
databaseUnavailable while MCP still returns it, path and all, in the JSON-RPC error -
pre-existing and untouched here, but the gap widened. And requiredValue rejects any
value beginning with `--`, so a db path starting with dashes is unreachable;
vanishingly unlikely, worth knowing.
The PR body was edited before merge to remove a closing keyword next to an issue
number, so ryanbr#1751 stays open for a deliberate decision rather than being actioned by
the merge.
Thanks @kvnloo - one dispatcher instead of two switch statements is the right shape,
and stating that local Swift execution was not performed, rather than implying it,
is exactly the disclosure that makes a review efficient.
A brief wake ended the night. Reported in ryanbr#1657: up once for the bathroom, straight back to sleep, and a 6h40m night scored 150 minutes at eff=0.96 - a confident, clean, wrong answer that then fed the 21-day baseline every later night is judged against. THE BRIDGE COULD NEVER REACH THE CASE IT WAS BUILT FOR. bridgeSparseSleep merged only runs already adjacent in its own output, so an intervening active run was appended first and made the next pair unreachable. A bathroom trip is definitionally an active run. The reporter's trace put a number on it: the bridge merged NOTHING on 14 of 14 sparse nights, and the pieces then died at the 60-minute session floor. One active run up to sparseBridgeActiveMaxMin is now absorbed, still subject to sparseBridgeGapMin and to the HR band; two or more consecutive active runs are not. THE HR GATE USED THE WRONG STATISTIC. hrSleepBandAcross judged on the MEAN, which a single arousal spike drags out of band - the exact statistic confirmSleepWithHR documents as wrong twelve lines below and uses the median for instead. Median now. It still rejects a SUSTAINED elevation, which is the discrimination the gate exists to make, and without it removing the active-run blocker would not have helped anyway. Four review passes, each earning its place. The bound started at maxGapMin (20), argued as principled reuse rather than an invented number. Every unit case passed. An end-to-end test through detectSleep did not: a FIFTEEN-minute interruption produces a TWENTY-ONE minute active run, because classifyStill smears the still/moving boundary and buildRuns closes at sample edges. The detected run is systematically longer than the interruption it represents, so a bound reasoned from how long someone is actually out of bed will always be too tight. 30 now, stated as a judgement rather than dressed as derived, and the end-to-end pair exists on BOTH platforms - that pair is what found this, and the smear is a property of the implementation, so a Swift-only divergence would otherwise leave the bound silently wrong on Apple. Swift's shadow tracer is gone. It kept a second copy of the bridge loop purely to emit trace lines, which would have had to be edited in step with this change; merge and trace are one pass now. Android had none of that machinery at all - the line that made this diagnosable was iOS-only, which is the only reason we know any of it. Two comments corrected rather than left: the bridge's own doc claimed intervening active runs were absorbed (they were not, and that sentence is much of why this went unnoticed), and the empty-attempts line no longer names a cause that has just been removed. One existing Swift test asserted "an intervening active run is never a considered pair" - true when written, and it WAS the bug; repinned to the same shape. An honest limit, in the PR body too: the absorbed minutes land in TST. Both stagers call wake primarily off HR, not motion, so an interruption the bridge absorbed had sleep-band HR by construction and will generally stage as light sleep rather than wake. The HR gate is what decides - movement with sleep-band HR is a stir scored as sleep; movement with elevated HR never reaches the bridge. Trading four lost hours for about fifteen generously-scored minutes is a far smaller error pointed the right way, but it is an error, not a clean win. An earlier draft of this branch claimed the span staged awake with no inflation. That was wrong and is corrected here. The parity pass found the trace lines byte-identical - and nothing keeping them so. Neither suite asserted the rendered line, only the fields behind it, so a format change on one platform would have diverged in silence. Both now pin the same whole string. NOT changed: minSleepMin stays at 60. The 45-57 minute runs it drops are real, but that is a separate judgement about what counts as a session and deserves its own evidence. With the bridge working, fewer runs reach that floor fragmented. Android 4,938 and StrandAnalytics 1,672 (one skip), zero failures. Doc lint and i18n clean. NOT validated on a real interrupted night - the reporter has not caught one with the trace on yet, so the end-to-end fixtures stand in for it.
Build numbers only - 10.6.1 stays the unreleased line, and 395 clears the staging high-water so the APK installs over 394. Carries the sparse-bridge change: a night interrupted by one short trip out of bed is no longer ended at the interruption. Anyone whose nights were being truncated that way should see fuller totals; the honest caveat is that the absorbed minutes land in sleep rather than wake, because both stagers call wake off HR and the bridge only absorbs spans whose HR was already in the sleep band. Also the ryanbr#1746 PENDING-ack change, so one "offsets may have moved" line per sync stops appearing on healthy 4.0 hardware, and the per-strap Last sync / Data write attribution from ryanbr#1745.
…anbr#1756) Found in the 13:17 capture, and it was my own scheduling rather than the strap. DIS reads and CCCD writes share ONE serialized GATT queue. The probe was posted on a fixed 6s delay, chosen on the reasoning that the DIS chain - posted at 3s - would be done by then. On one link it was still running at 7s: 07:57:06 writeDescriptor busy for fd4b0003 ... retry 1/8 07:57:06 ... 8 busy retries across the four chars, shared budget exhausted 07:57:06 writeDescriptor rejected for fd4b0003/4/5/7 (gave up after 8 retries) 07:57:06 Unbonded offload probe: none of the 4 ... completed their subscribe, and none was refused either ... proves nothing either way 07:57:07 DIS: modelNumber=MG <- the chain, still going Not one descriptor write reached the air. Reasoning about a delay lost to measuring one. The probe now waits on the actual signal, checked once a second and bounded at eight checks, then goes anyway and says which happened. The same log also handed over a control case that ryanbr#1749 asserted without one. Every link whose CCCD writes reached the air died at 10.8s - 4.8s after the probe, by localTerminate, the CLIENT_HELLO's own signature. The ONE link where all four writes were abandoned locally lived 31.3s and died of an ordinary connectionTimeout. The teardown tracks the write reaching the strap, not the probe running. Three review passes, and all three found the same shape of defect: a flag claiming something that had not happened. 1. the contention itself 2. disChainInFlight was set BEFORE readDisIdentity, which has five early returns above the read it issues plus an unavailable-characteristic branch - so on any of those the probe stood aside for a chain that never started. readDisIdentity now reports whether it ISSUED a read, and the flag is set from that answer rather than from the intention to ask. 3. readNextDisExtra is only reached from the SUCCESS path, so a REFUSED read ended the chain while the flag stayed set - and ryanbr#490's whole subject is a strap that refuses DIS, so that was the case the probe most wanted a clear queue for. All three were survivable because the cap bounds them, which is exactly why they would have gone unnoticed: a wrong wait that self-corrects looks like a working wait. The wait decision also moved into a pure helper beside the rest of this file's judgements. It was inline in the client - argued for in a comment and asserted nowhere - which is precisely how ryanbr#1755 shipped a bound every unit test agreed with and the real pipeline rejected. Parity checked rather than assumed: the DisUnbondedRead pure twin pair is untouched, four functions each side, and Swift performs its DIS read inline with no readDisIdentity counterpart, so the Bool return has no twin to keep in step. That structural difference predates this work. Everything else is the probe, Android-only by design. Staging 396 / iOS 277. 4,941 tests, zero failures. Doc lint and i18n clean. The next capture is what confirms the probe now starts on a clear queue.
The Coach kept answering today's question inside yesterday's conversation. CoachViewModel._messages is in-memory and only reset by clearing the API key or switching provider, and the ViewModel outlives a night - Android keeps the process around for days. The DATA was never stale: buildContext() re-reads the store on every send. It is the assistant's own earlier turns stating yesterday's figures, and the model staying consistent with them, which reads as "the coach only talks about my imported data" after a night of fresh strap data. Force-quitting was the only cure. MAX_STORED_MESSAGES already bounded the transcript's SIZE for the same long-lived-session reason; this bounds its AGE. isStaleConversation is strictly forward (`>`, never `!=`), so a clock moving BACKWARDS - flying west, a timezone change, an NTP correction - cannot wipe a conversation the user is mid-way through. @dwehrmann mutation-verified that: relaxing it to `!=` fails two of the six cases, so the test has teeth on the subtle part rather than pinning a tautology. Better than the PR claims, and worth recording. injectContext prepends to indexOfFirst { role == "user" }, so after the clear the new turn IS that first turn and the context lands at the top of a fresh conversation. Previously it sat in yesterday's opener, buried under a night of turns. This improves where context goes, not only how recent it is. Checked as a stale PR rather than trusted: opened 22 August, main 226 commits ahead, and its CI ran when only three checks existed. Merged locally onto current main - clean, 4,947 tests, zero failures, CoachConversationDayTest 6/6, doc lint and i18n clean. One inconsistency left alone: conversationDay is not reset at the two existing _messages.value = emptyList() sites. Harmless - a stale day just clears an already empty list - but the field outlives what it describes. Thanks @dwehrmann. The parity gap was flagged rather than papered over, with the reason - app-target Swift needs Xcode, and a compile error there passes every green check. That is the right call, and the Swift twin follows separately now that it can be compiled and run.
The Apple half of ryanbr#1542, which @dwehrmann deliberately left out rather than ship Swift they could not compile. That was the right call, and this is the evidence: it did NOT compile on the first attempt, and nothing in default CI would have said so. error: call to main actor-isolated static method 'isStaleConversation(lastEpochDay:todayEpochDay:)' in a synchronous nonisolated context AICoachEngine is @mainactor so its @published mutations stay on the main thread, and the statics inherited that isolation. Marked nonisolated rather than pushing @mainactor onto the tests: they are pure functions of their arguments, touch no published state, and the Kotlin twin is a plain companion object for the same reason. The iOS leg had passed on the broken commit - only the macOS leg compiles StrandTests - so a compile-only check would have called it green. Same bug, same rule. messages is @published and held for the app's lifetime, so the coach answered today's question inside yesterday's conversation; the data was never stale, it is the assistant's own earlier turns stating yesterday's figures. isStaleConversation is strictly forward (`>`, never `!=`) so a clock moving BACKWARDS cannot wipe a conversation mid-use, and the check sits after the guards so a send that never happens leaves the transcript alone. One thing is Swift-only because Swift-only needs it. Kotlin gets its epoch day from LocalDate.toEpochDay(); this side computes one, and the obvious way - dividing a startOfDay interval by 86,400 - is wrong, because a day is not always 86,400 seconds. localEpochDay uses calendar day arithmetic. The re-review then caught that the DST case written first was spring-forward, which is the obvious one and not the dangerous one: a 25-hour autumn day is where truncating whole days from a non-midnight origin could put two local midnights in one bucket, and a transcript would then survive a night once a year. Both directions pinned now, plus a zone west of UTC where the epoch instant falls on the previous local day. Also folded in, on request, and checking it corrected the claim that prompted it. I had said Android wipes the transcript on a PROVIDER SWITCH and Apple does not. Wrong - neither does, so that is shared behaviour rather than a divergence, and whether provider A's history should reach provider B is a real question but the same one on both platforms. The actual divergence was narrower: Kotlin's clearKey and disconnect have always emptied the transcript and the Swift twins never did, so on Apple "clear my key" removed the credential and kept the conversation, and disconnecting returned the user to the setup screen with the whole conversation behind it. Both retire it now, and conversationDay goes with the transcript on BOTH sides - Kotlin was not resetting it, which is harmless but is a field claiming something untrue. Nine Swift cases: six mirroring the Kotlin suite one-to-one, three Swift-only. app-build green on both legs and its Strand leg RUNS StrandTests, so they execute rather than merely compile. Android 4,947 tests, zero failures. Doc lint clean. Staging 397 / iOS 278.
…yanbr#1635) A field capture on 31 Aug (build 393, whoop-MGB…, fw 50.41.1.0) shows the unbonded offload probe starting 18 times across 24 connects, each start writing the puffin CCCDs and each link dying ~4.8s later — an endless reconnect loop and a permanent "Reconnecting to your WHOOP" notification. The control case sits in the same log: the one link whose four CCCD writes were all abandoned locally lived 31.3s and ended in connectionTimeout, while every link whose writes reached the air died at 10.8s, locally terminated. UNBONDED_PROBE_MAX_SILENT_LINKS exists for exactly this, and it worked — within one process. Nothing bounded the processes. The foreground service restarts, the counter went back to zero, and the probe bought three more link-killing attempts on a strap that had already answered the same way three times. The file's own doc admitted this ("the honest limit of an in-memory bound") without following the admission anywhere. The budget is now persisted per device, and lives on PuffinExperiment rather than beside the refusal latch in NoopPrefs. That placement is load-bearing: turning the switch on has to hand the budget back, the setter has no device in hand so it clears by prefix, and a SharedPreferences sweep can only reach its own file. Written to NoopPrefs and swept from noop_experiments, re-enabling the switch would have cleared nothing and done it silently, the give-up line having already latched. Keeping the budget on the object that owns the switch makes that drift unrepresentable rather than merely documented; a test pins the key against the prefix as well. The re-arm is sampled in the setter and not at connect, deliberately: once the probe retires the link is stable, a stable link produces no connects, and a switch flipped off and on while idle would never be seen. It fires on the off->on edge and not on any write of true, so a caller rewriting the current value cannot hand back three more link-killing attempts through the mechanism meant to bound them. Both charge sites — "asked and heard nothing" and "link died mid-probe" — now share one path. A quiet link and a link torn down by the very subscriptions we wrote are the same evidence about this strap, and counting only the first is how 18 starts fit into 24 connects. Two review passes also corrected the retirement line, which claimed the strap "serves those characteristics unbonded". That holds only for links that subscribed and then stayed quiet; a link lost mid-probe confirms no subscribes at all, so on this capture — where every charge was a lost link — the line would have recorded the opposite of what the strap demonstrated. It now names both ways the budget can be spent and asserts neither, the per-link lines still carrying the detail. Docs that described the superseded design were corrected with it, including the switch's own promise that opting in "costs one link and not a loop" while it cost 18. Behaviour: three links per strap ever, rather than three per process; nothing after a service restart; another try by switching off and on instead of clearing app data. UnbondedOffloadProbeTest 33 tests, whole com.noop.ble suite 604, zero failures on a forced clean run. compileFullDebugKotlin, doc_comment_lint and i18n_audit clean. Android only; there is no Swift probe to twin. Refs ryanbr#1635.
…yanbr#1635) A capture on 31 Aug (build 397, 5/MG, fw 50.41.1.0) has two disconnect bursts sixteen hours apart, and the gap between them is the point: the link was perfectly stable all night, so the strap is not what is unstable. 00:39:38 -> 00:41:20 7 links, all ~10.8s the unbonded offload probe 00:41 -> 17:21 16 hours, zero disconnects 17:32:05 -> 17:39:44 18 links, all 4.8s the CLIENT_HELLO The second burst should have ended itself. At 17:32:48 the give-up fired exactly as designed - "never acknowledged the secure handshake 5x in a row, leaving the handshake off so live heart rate keeps streaming" - and then 13 more CLIENT_HELLOs went out. The line printed when a hello is actually skipped appears zero times in the whole log. With "Ask Android to pair" on, the explicit-bond request cleared the suppression latch. The comment there argued this was safe because it happened "ONCE, here", as against being re-armed by "an OS pairing exists", a condition that never goes away. It guarded the wrong recurrence: shouldRequestExplicitBond is bounded only by alreadyRequestedThisLink, a per-LINK flag, and on a strap answering SMP "Pairing Not Supported" neither bond condition ever becomes true. So createBond runs on every connect and the clear ran with it. The capture holds 18 pairing requests and 18 hellos - one of each on every link, which is exactly the pairing helloDeferredByExplicitBond exists to forbid. Nor could it recover. recordRefusal() reports the crossing exactly once and stays gaveUp until reset, so the latch is written once per session; a clear on a recurring event does not cost one link, it costs the latch permanently. The designed end state - live HR, handshake off, link stable - was unreachable while that switch was on. The clear is removed. Recovery is the explicit Connect the epitaph already names. The encrypted-bond clear is deliberately not offered as an automatic alternative: it lives inside the hello write-completion callback, so a suppressed hello cannot reach it, and three separate review passes were needed to stop that overclaim reappearing - once in the production comment, once in the test comment written to correct it. All three clears are now named with the one suppression puts out of reach marked as such. The pairing experiment is untouched: createBond still runs on every link, it just no longer drags the hello along with it. Parity checked rather than assumed. Swift has the same give-up mirror and the same latch; its three writes are the release path, the give-up write, and one clear gated on ClientHelloOutcome.isAck - an outcome, not a request. No twin exists, and could not: CoreBluetooth has no createBond. Tests pin the contradiction from both sides. ExplicitBondTest: the permanent state of a refusing strap still returns true and the only input that returns false is the per-link flag, so nothing may hang a once-only side effect off this request. HelloSuppressionTest: the give-up reports its crossing once, so the latch is written once and a recurring clear is permanent. 606 tests in com.noop.ble, zero failures on a forced clean run; compileFullDebugKotlin, doc_comment_lint and i18n_audit clean. What this does not settle: with the hello suppressed, createBond still runs once per link on a strap that refuses SMP. Nothing in this capture says whether that alone keeps a link up, because the sixteen stable hours contained no connects at all. If a loop survives this, that is the remaining cause and it will be readable for the first time. Android only. Refs ryanbr#1635.
Build numbers only - 10.6.1 stays the unreleased line, and 398 clears the staging high-water so the APK installs over 397. Carries both loop repairs. The unbonded offload probe's silence budget now survives a service restart, so a strap that has answered three times the same way is not asked again on the next launch; and asking Android to pair no longer clears the hello give-up on every link, which is what kept the handshake going out after the app had decided to stop. For a 5/MG that refuses SMP the expected end state on this build is a stable link with live HR and the handshake off, rather than a reconnect every ten seconds. Tapping Connect still retries the handshake, and turning the offload experiment off and on hands the probe a fresh budget.
…br#1635) Asking Android to pair was the only probe in this area with no bound at all. The CLIENT_HELLO has a give-up after five refusals, the unbonded offload probe has a persisted three-link silence budget, and this one had nothing: gated only by optedIn, family, the two bond flags, and alreadyRequestedThisLink - a per-LINK flag. On a strap answering SMP "Pairing Not Supported" neither bond flag ever becomes true, so it fired on every connect indefinitely. That is worse than a quiet loop. Every declined request surfaces a system "Pairing rejected" notice, so the cost lands on the user rather than in a log nobody reads, and it was reported from the field as exactly that. The request now stands down once the bond give-up has latched for the device. The signal is the PERSISTED suppression latch rather than BondRefusalGiveUp.gaveUp, which lives in one process - a bound that dies with the process is the same defect corrected twice already this week, in ryanbr#1761 and ryanbr#1762. Gating one experiment on the other is deliberate. BondRefusalGiveUp does not track the hello; it tracks whether this strap will form an encrypted bond, which is the single thing both routes are trying to achieve and neither has. By the time it latches, the pairing request has had its five links and returned BOND_NONE every time, with an HCI capture showing SMP 0x05 underneath. The experiment concludes; it is not cut short. It ends the loop, not the experiment. Both things that clear the latch are user actions that could have changed the answer - tapping Connect, and forgetting the device - and putting a 5/MG into pairing mode then tapping Connect is the one flow known to have worked on real hardware. The trade is that a user who enters pairing mode and waits is no longer asked automatically; with the link now stable after ryanbr#1762 that path was already unreliable, since it only fired when a new link happened to form. Nothing here touches connecting. The give-up this rides is the suppress branch, which stays connected and keeps live HR streaming; the pause branch belongs to an auth refusal and is untouched. The diff contains no connect, reconnect, scan or pause line. The latch is read once, above both decisions that consult it, because the pairing request is made before the hello is considered. That single read is only correct while nothing between the two writes the latch - true since ryanbr#1762 - and the code now says so, with what to do instead. A one-shot line reports the stand-down: a switch that is on and silently doing nothing reads as a broken switch, and the user has just been reading Android's rejections. The line claims the retirement rather than a per-connect skip, matching its two siblings, and the branch that emits it asks the gate itself whether the give-up was the reason rather than restating half the gate's conditions - which would drift into blaming the give-up for some later check's decision. Forgetting a device re-arms it, as clearPairingHint already did. Field evidence, build 398, same strap, after the two earlier repairs: refusals climb 1 to 5 across five links, the give-up latches at 18:45:23, the hello is suppressed eight seconds later, and afterwards there are zero hellos and zero disconnects - one further pairing request, which is what this removes. The UI reached "Live HR (not fully paired)", the ryanbr#1635 end state. ExplicitBondTest +2, 608 tests in com.noop.ble, zero failures on a forced clean run. compileFullDebugKotlin, doc_comment_lint and i18n_audit clean. Parity: none owed, and checked rather than assumed. All three ryanbr#1635 bond experiments - noopWhoop5ExplicitBond, noopWhoop5UnbondedOffload, noopWhoop5HelloDespiteRefusal - exist only on Android, and Swift has a single consumer of the suppression latch, so there is no second decision to hoist a read for and no gate to add. CoreBluetooth exposes no explicit pairing API, which is why this is permanently one-sided. Refs ryanbr#1635.
Build numbers only - 10.6.1 stays the unreleased line, and 399 clears the staging high-water so the APK installs over 398. Carries the last of the three unbounded ryanbr#1635 experiments. Asking Android to pair now stands down once the strap has refused the encrypted bond enough times for the handshake to be latched off, so a 5/MG that answers SMP "Pairing Not Supported" stops producing system "Pairing rejected" notices the user did not ask for. Nothing about connecting changes: the give-up this rides keeps the link up and live HR streaming. Tapping Connect asks again on the next link, so putting a strap into pairing mode and reconnecting still works; only the automatic retry on every link is gone, and with links stable since 398 that path fired only when a new link happened to form.
…1951) Option 1 from ryanbr#1948, and the investigation made it bigger than the issue said. ## What was happening The keep-alive polled battery commands the 5/MG send allowlist rejects before they leave the app, under a comment claiming the pack "rides the SAME cadence as the strap's own gauge". Android sent `GET_BATTERY_PACK_INFO`, which the allowlist admits only while a user-initiated probe is in flight: ``` 12:31:48 send(GET_BATTERY_PACK_INFO) skipped — no WHOOP 5/MG framing for this command yet 12:32:48 send(GET_BATTERY_PACK_INFO) skipped — no WHOOP 5/MG framing for this command yet ``` 40 of them in 40 minutes of one capture: one wasted call and one skip line per minute. **iOS was worse than ryanbr#1948 recorded.** The allowlist has no clause for `.getBatteryLevel` either, so the whole block was dead rather than half of it: TWO sends and two skip lines per tick. The issue said "one refused command per platform"; that was wrong and is corrected there. A re-review then narrowed WHO pays it. `keepAliveMayRun` refuses an unbonded 5/MG outright, so the tick never fires there and only a BONDED 5/MG was paying. That correction strengthens the case rather than weakening it, and is why the comment now argues from co-residence: the same `didBond` that lets this tick run has already fired `enableLiveNotifications` a few lines above, which drives the `0x2A19` read a 5/MG's percent comes from. The read and the refused send were always on the same tick, so dropping the send cannot strand the reading. ## The change Android drops the pack poll from the WHOOP5 keep-alive branch. iOS makes that whole battery block WHOOP 4.0 only, which matches `refreshBattery`, already guarded `if selectedModel.deviceFamily == .whoop4` for the same command. Nothing is lost: - the strap's PERCENT comes from the `0x2A19` read both platforms already drive off this same keep-alive, on the same tick the removed send rode (iOS throttled to `whoop5BatteryReadMinIntervalSeconds`) - the PACK's charge comes from the pushed pack-info event (109), that flag's only writer since ryanbr#1945 - Android's user-initiated probe path is untouched, and remains how the hardware question ("does a 5/MG answer 151 at all?") gets asked On iOS this leaves opcode 151 with no sender at all, because there is no iOS twin of that probe. That is not a loss: it restores exactly what `FrameRouter` already records for its own decoder, "nothing sent the command, so the decoder had no caller". Noted at the call site so the asymmetry is not rediscovered. The comments now say what actually happens instead of describing a cadence that never ran. ## Tests Both sends live in keep-alive bodies no unit test can drive, so both are pinned against the source. The Android guard is scoped to the WHOOP5 branch and also asserts the WHOOP4 poll beside it survives, since this is a 5/MG-only removal rather than a change to battery polling. Verified as a NEGATIVE CONTROL rather than assumed: re-adding the Android send fails the guard, restoring it passes. The Swift guard needed a fix a re-review caught before CI did: it asserted the block contains no `getBatteryPackInfo` without stripping comments, while the comment above it names that symbol to explain why it is not sent, so it would have failed on its own rationale. It judges the code only now, the rule the Kotlin twin already followed. The Swift guard normalises whitespace on both sides, because a Swift multiline literal strips indentation to its closing delimiter and so can never carry the source's own leading spaces — a raw comparison would have failed for a reason unrelated to the guard. Android suite 5521 green, doc-comment gate clean. The Swift guard DOES run on this PR: `app-build.yml` triggers on `Strand/**` and its macOS leg has a `Test Strand` step, so `StrandTests` executes in CI here. Worth flagging separately: that path filter lists `Strand/**`, `StrandiOS*/**`, `Packages/**` and `project.yml`, but NOT `StrandTests/**`. A PR touching only app-target tests therefore builds and runs nothing, and a broken test file would merge unverified to fail on the next PR that happens to touch `Strand/**`. This PR is covered because it edits `BLEManager.swift` too. ## Risk About as low as it gets: these are sends the allowlist already refused. The visible effect is less pointless traffic and quieter captures — an iOS Test Centre connection capture on a 5/MG was carrying roughly two wasted lines a minute.
Groundwork for ryanbr#1954. One line in the trigger, no new job. ## The gap `app-build.yml` runs the app-target tests on its macOS leg: ```yaml - name: Test ${{ matrix.scheme }} if: matrix.scheme == 'Strand' ``` but its `paths:` listed the sources and not the tests. So a PR touching only `StrandTests/**` started nothing: a test that failed to compile, or simply failed, merged unverified and then went red on the next unrelated PR that happened to touch `Strand/**`. The mis-attribution is the part that costs time. The PR that turns red is not the one that broke it, and the person reading that failure has no reason to suspect a test file merged days earlier. `project.yml` was already in the list but does not cover this. The target takes `sources: [StrandTests]`, a directory, so adding a test file changes no other tracked path. ## Why it is worth a line `StrandTests` holds the app-target logic the Swift packages cannot reach — backfill continuation, scheduled-report and strain-target policy — and the source-guard tests that pin call-site invariants no unit test can drive. Adding one of those is a routine change and exactly the shape the filter missed. Found while confirming a new guard test in ryanbr#1951 would actually run in CI. It does, but only because that PR also edits `BLEManager.swift`. ## Scope `StrandTests` is the only app-target test directory in the repo, and `app-build.yml` is the only workflow listing `Strand/**`, so there is no sibling with the same gap. A re-review checked that claim against the other suites rather than assuming it, and the answer explains the bug: Android CI triggers on `android/**` and its tests live inside that; the package suites live under `Packages/**`. Both are already covered by their subject's glob. `StrandTests` is a SIBLING of `Strand/` rather than part of it, which is why it alone fell outside every path in the list. That is now in the comment, so whoever adds the next test target knows which shape needs a trigger entry and which does not. Costs macOS minutes only on PRs that genuinely change those tests. This PR edits the workflow, which is itself in the paths, so the change verifies itself.
Groundwork for ryanbr#1957, Android only. iOS is deliberately left out — WidgetKit can draw the trace directly, so the renderer will not port, and the shared half is worth agreeing on here first. ## Why it is shaped this way Glance compiles to RemoteViews, which has no Canvas. A sparkline cannot be drawn by a composable at all: it has to be rendered to a Bitmap and handed over as an `Image`. So the work splits, and the split is the design: - **`HrTrace`** — everything decidable WITHOUT a Canvas. Retention (one point per minute over three hours, capped), encode/decode, stats, normalisation into the pixel box, and the tick choices. Pure, 16 tests. - **`HrTraceRenderer`** — deliberately stupid. `moveTo`/`lineTo` over coordinates handed to it. It chooses no tick, no range and no point, because a Bitmap is not something a JVM test can assert about and as little as possible should depend on one. The LABELS are not in the bitmap either, though the design has them. They are Glance `Text`, so they stay crisp at any density, themed light and dark with the rest of the widget, and legible to TalkBack — none of which a baked-in label would be. RemoteViews carries its payload over a Binder transaction with a hard size limit, and a widget that exceeds it does not degrade, it fails to render. Hence the bitmap cap. ## No producer changed `WidgetSnapshotStore` folds each live sample into the trace in `save` and returns it pruned from `load`, so `WhoopConnectionService` and `AppViewModel` did not have to learn about it. That is affordable because `PushGate` already throttles an unchanged key to about once a minute, which is exactly the bucket size the trace keeps — the read-modify-write runs about once per point, not once per sample. `PushGate`'s key is explicit field concatenation, so adding a field to the snapshot does not perturb throttling. ## The tested cases are the ordinary ones Not exotic — these are what a home screen actually does: - a widget placed mid-afternoon has exactly ONE point: it draws a dot, rather than an empty chart that reads as "no data" - a resting arm holds one bpm for minutes: a zero range runs down the vertical MIDDLE, not pinned to an edge where it would read as a flatlined or maxed-out heart - a backwards clock cannot grow the series past its cap - a malformed stored series degrades to a partial trace instead of throwing on the render path, where the alternative is a crashed home screen - time ticks anchor to the DATA, not the window, so forty minutes of history does not get labelled with two hours that were never sampled ## Two choices worth vetoing - The trace is **red**, the app's heart colour, rather than the blue of the design that prompted this. - **4x2 cells**, wider than the 2x2 score widget, because a trace needs horizontal room to be readable. ## Verification Full Android suite 5537 green, i18n audit clean, doc-comment lint clean. Four new strings across all seven Android locales — a `GermanLocalizationTest` failure is what caught that the set is seven, not the four I had assumed. **Not verified on a device.** The bitmap path is exactly the kind of thing that looks right in code and wrong on a launcher, so this wants a testing build and a home screen before it is trusted. ## What review passes changed after the first push Five, and none of them were reachable from the tests, which is the useful part: - the bitmap caps were per-DIMENSION, so 1080x480 was legal at 1.98 MB against a Binder ceiling near one; and the geometry ran at the requested width while the renderer clamped its own, silently clipping the right of the trace. One box, decided on bytes, now serves both. - `timeTicks` was written, documented and TESTED, then never called. The design has time labels and the widget did not draw them. - the bpm scale was stacked from the top with fixed gaps, so it was three numbers near a chart rather than a scale aligned to it. - the empty state reserved chart height to draw nothing, which on upgrade is a blank rectangle for the first few minutes. - TalkBack read the widget as noise, and staleness was carried ONLY by a dimmed colour, so a carried-over reading was announced as current. Fixed the way the sibling widget settled the same thing in ryanbr#1799. The i18n echo gate then caught Min/Max being identical to English in de and fr. Running `Tools/i18n_audit.py` bare exits 0; the workflow runs it with `--ci` and a base ref, and only that applies the echo and coverage gates.
Follow-up to ryanbr#1958, from putting it on an actual home screen. Three defects, none of which a test could have shown. ## The chart did not span the card Everything sat in the left portion with dead space beside it. The cause: I sized the chart from `LocalSize.current`, and a One UI launcher reported a size smaller than the card actually occupied, so every width derived from it was short. The chart now takes the row's remaining width by weight. The bitmap is still measured in pixels — it has to be — but only to be drawn and then stretched, which a smooth line survives without visible cost. Layout is the launcher's business, not arithmetic of mine. `LocalSize` being a hint about the cell rather than a measurement of the card is the mistake underneath all of this, and I had treated it as the latter. ## Labels that said nothing A single reading rendered a bpm scale of `78 / 78 / 78`. Honest — with no range there is nothing to scale against, which is the behaviour a test pins — but three labels repeating the headline are noise. The scale now appears only once there is a range to describe. The time axis went the same way. One instant pinned to the left edge read as a stray rather than an axis, so it waits for a span to label. ## A void under the stamp A 4x2 cell is taller than this content, so "Updated" floated with empty card beneath it. It is pinned to the bottom now and reads as a footer. ## Then: the graph was still small Two things, and the second was self-inflicted. The chart was pinned at 56dp while the card had roughly 36dp going spare — and the spacer added above to pin the stamp to the bottom was eating exactly that slack. The fix for the void consumed the room the graph needed. The chart takes the leftover height itself now, about 64% taller. Taller then collided with the payload budget: at four bytes a pixel a 92dp chart left only 518px of width for an ~834px card, a 1.6x horizontal stretch — the very upscale removed a commit earlier. They stop competing once the bitmap stops paying for an alpha channel it never uses. The trace is one hue over an opaque card, so it is drawn RGB_565 over the card colour, gradient fading to that colour rather than to transparent. Half the bytes, so the same budget affords 1034px at that height: a 1.24x DOWNSCALE. Dither is on, since 565 bands a smooth ramp. ## And the assumption underneath all of it Glance's `Image` defaults to `ContentScale.Fit`, which preserves aspect. Every sizing decision above assumes the opposite — width drawn with headroom SO THAT it downscales, height drawn to an estimate because a weighted box has no knowable size. Under `Fit`, a 1034x253 trace in an 834x253 box would letterbox to 834x204 and sit 49px short, silently undoing the height it had just been given. The arithmetic in both commits was right and checked. The assumption underneath it lived in a library default I had never written down, so the two fixes were correct and jointly inert. Now `FillBounds`, verified against the Glance API rather than assumed. ## Verification Full Android suite 5544 green, i18n `--ci` clean, doc-comment lint clean. The pure-half tests are untouched and still describe the same behaviour: what changed is which of it gets RENDERED, not what it computes. Wants another look on a launcher, and specifically once the trace has more than one point — the single-reading state is the only one seen so far.
iOS twin of the heart-rate widget (ryanbr#1957), Android having landed in ryanbr#1958/ryanbr#1961. ## What ports, and what deliberately does not `HrTrace` ports whole — retention, normalisation, stats, tick choices. Those are the same decisions about the same heart, and a trace that kept a different window or picked different labels would be a different READING, not a different look. Sixteen Swift tests mirror the Kotlin ones case for case, so a change landing on one side alone fails. The DRAWING does not port, and should not: | | Android | iOS | |---|---|---| | renderer | Bitmap under a byte budget, RGB_565 | stroked SwiftUI `Path` | | why | Glance compiles to RemoteViews: no Canvas, Binder payload ceiling | WidgetKit is SwiftUI, resolution-independent | | sizing | `fitBox` / `widestAtHeight` / headroom | geometry gives the box | So `fitBox`, the byte budget and the pixel depth stay behind — they answer a question this platform does not ask. `encode`/`decode` stay behind too: the snapshot here is already Codable and carries `[HrPoint]` directly, where Android packs a SharedPreferences string. ## A defect found before it shipped `save()` only ran when `renderedContentChanged` was true, and that compares bpm, not history. A steady heart — the ordinary case at rest — would have recorded no point, so the trace would stop advancing while the strap streamed happily and prune itself to empty. `traceNeedsPoint` is the twin of Android's `PushGate` re-admit, keyed on the minute BUCKET so it asks for a write exactly when `HrTrace.append` would record one and never more often. It persists without a reload, since spending a WidgetKit reload a minute is what the dedup above it exists to avoid. ## The accent `StrandPalette.zone5`, which resolves to `#C84E1E` / `#E0662F` — exactly the hexes the Android widget carries as a local mirror. That was luck rather than judgement: the colour chosen by eye there turns out to be the palette's HR zone-5 token. Both sides now record the match, so neither drifts. ## Accessibility SwiftUI can combine the card into one spoken sentence, which Glance cannot — Android had to settle for per-label descriptions because it offers no way to mark a `Text` decorative. So this reads as one label rather than a run of loose numbers. Staleness is not colour-encoded here at all, so nothing needs saying about it. ## What review passes added after this was opened Four gaps, each found by comparing against the Kotlin side rather than by reading the Swift in isolation: - **Spoken text was hardcoded English.** The audit does scan `.accessibilityLabel(`, but it matches a literal immediately after the paren, and these were built in a computed property — so extracting them for readability is what hid them. Worse, after switching to `String(localized:)` the audit stayed green with the keys still ABSENT from the catalog: "is it extracted" and "does the key exist" are different questions and it only asks the first. Three keys added across ten locales. - **The trace was not pruned on read.** Android prunes on the way out as well as in; this drew points that had aged out, under a time axis implying they were current. Measured from `entry.date`, since WidgetKit renders an entry at ITS date. - **No age rule on the headline.** Android drops a reading past fifteen minutes and dims one past two. Pruning made the absence incoherent rather than merely wrong: an empty chart under a confident number. `HrDisplay` ports with the same thresholds, anchored on the newest TRACE POINT rather than `updated`, because a publish fires for battery and score changes too. - **Work done twice, on both platforms.** Android sorted in `append` and again in `prune`, and scanned for `stats` once per composable; iOS decoded the App Group snapshot twice per publish. Same shape each time: a helper written to stand alone, called from somewhere that had already done the work. ## Verification Android suite 5544 green, i18n `--ci` clean, doc-comment lint clean. Two catalog entries added across ten locales, TEXT-SPLICED rather than JSON round-tripped since the file is ~6MB — and Italian needed real abbreviation periods to clear the echo gate, the same way German and French did on the Android side. Swift is CI-verified as usual. `StrandTests` runs on the app-build macOS leg, and this PR touches `StrandiOSShared`, `StrandiOSWidgets`, `StrandiOS`, `StrandTests` and `project.yml`, so that job triggers. **Not verified on a device.** The Android one needed three rounds on a real home screen after passing every gate, and nothing here has been near a launcher.
…yanbr#1959) Closes the iOS half of the keep-alive divergence: an unbonded 5/MG now gets the same periodic 0x2A19 battery read Android has had, at the same cadence. Verified rather than taken on trust: - the insert lands immediately before `guard didBond else { return }`, so it runs for unbonded straps just before that guard cuts them off - `!didBond` in its own condition stops a bonded strap reading twice, since that one is already served by `enableLiveNotifications` - `shouldPollWhoop5Battery` already existed with `charging` defaulting false, so this reuses the throttle rather than introducing a second timer that could drift from the ~60 s Android cadence - `properties.contains(.read)` is checked before reading iOS-only is correct: the gap was this platform lacking what Android already did, so there is nothing to mirror back. CI green on all four checks.
…yanbr#1963) The backfill deferral diagnostic explained the dead end accurately and then stopped, without naming the one action that exists for it. It names it now. Verified rather than taken on trust: - the hint is inside the `family == "WHOOP5" && !didBond && !helloEverWrittenThisLink` branch, so it cannot appear on a 4.0 or on a bonded strap — it is scoped to the same structural claim the surrounding text makes - the three-way split is right: silent when the probe is already on (its own lines report what it found), the retry path when it has retired, the toggle otherwise - both new parameters default to false, so existing callers and tests are unaffected - a log line, so no localization is owed The retired branch stays useful whatever happens to the local-teardown work in flight: if a false negative can retire the probe, telling someone how to un-retire it is exactly the right advice.
…mpty-offload banner (ryanbr#1965) The empty-offload banner was gated only on zero persisted rows, so it blamed the clock on a strap whose flash cursor was demonstrably healthy — advancing write pointer, drift of a second, every page carrying console output. Charging that strap changes none of it. Verified rather than taken on trust: - `sawNoFlashCursor` is an accessor over the existing private `loggedNoCursor`, which is reset in the PER-SESSION block on both platforms alongside `sessionMotionRows` and friends. That is the right scope: the banner is about THIS sync, and a strap that once lacked a cursor must not keep being told so - the priority is unchanged — `staleRecordBanner` still wins when the strap's own newest record dates the silence; the new split only replaces the fallback - both platforms move together, with tests either side - the copy carries no em-dash, and there is a test asserting it, which is the project rule Not localized, matching the sibling `lastSyncError` strings it sits beside. The PR says so explicitly rather than leaving it to be noticed, which is the right way to inherit an existing gap.
…pdates that change nothing A field report that battery drain feels worse with the HR widget placed, which three exports could neither confirm nor refute: "widget" appeared zero times in a full diagnostics bundle. This measures it, and takes the two savings that turned out to be real. Counters for pushes admitted vs offered, trace draws with their bytes and draw time, and redundant redraws. The HR widget is the only one that ships a bitmap rather than a few KB of text, so the payload is what a drain question turns on. Two savings. Providers with nothing placed are no longer updated. And a push is no longer sent at all when nothing the widgets display has changed - the Android half of what WidgetPublish.saveAndReloadIfChanged has done on the Apple side since the HR widget landed. Deliberately NOT the Apple rule, which also declines when only the trace advanced: WidgetKit rebuilds a timeline on its own schedule so a withheld point still reaches the screen, and Glance has no such rebuild. That variant waits for the counters to price it. Also records that WIDTH_HEADROOM never takes effect - the byte budget binds first at every realistic size, and at 480dpi a wide card is drawn NARROWER than it is displayed, so the bitmap is upscaled, which is the artefact the constant exists to prevent. That closes off the obvious lever rather than opening it. No cadence change and no visual change.
…ers an iOS twin The first capture from a device reported 215/h against a steady state of about sixty. The refresh clause is 60 seconds, so at most two of those six pushes could have come from it; the rest were the snapshot's fields arriving one at a time at launch, each a key change admitted on the spot. A minute of uptime was not a long enough floor because the problem is not sample size - the early samples are a different population. Rates now run over a steady window that opens a minute after the first push and are withheld until it is five minutes long, and the line says which window it is waiting on rather than implying a claim about uptime. Three further faults, all the same shape and none of them arithmetic: a counter whose name claimed more than its wiring delivered. Pushes the rendered gate declined were counted as sends, so the figure the whole drain question turns on could not fall when the gate did its job. Pushes with no widget placed were counted as sends too, at a second early return one further up - and that is the worst place for it, because an export taken with the widget REMOVED is half of the comparison the counters exist for. Both halves would have read alike. And the claim that iOS was not applicable was wrong. The push accounting maps exactly: the same throttle, the same reload-or-decline, the same diagnostics header block. Only the bitmap half does not, since that widget draws a SwiftUI Path with no Binder transaction, and it is left out rather than faked. iOS also carries a cost Android does not - WidgetKit budgets timeline reloads, and spending that budget makes the widget go STALE rather than costing battery - so the rate there counts reloads, not publishes. The twin then reproduced the very defect just fixed on the Android side: iOS never asks whether a widget is installed before reloading, so reloads were counted with nothing to receive them. Recorded honestly rather than gated, since the measurement was the defect and gating would be a behaviour change on a platform not testable here. Android: full suite 5,581 tests, 664 classes, 0 failures. Swift compiles on both build legs; StrandTests runs only in the on-demand app-build workflow, so the eight iOS tests are not exercised by default CI.
The recovery trace was reported MISSING on every capture that had one. The killer token was "charge score=", but IntelligenceEngine re-emits every recovery line as `charge day=<day> ` plus the body, so the bare token cannot occur in any report. Two bundles in hand carry 1020 and 816 of those lines and both said the trace was absent. The token is now the day prefix, the one part every variant shares, so it cannot go stale the way a token naming one variant did. Apple had the identical defect, naming three variants none of which could match. "Data write: no rows ever persisted" reads as every write and is stamped only when a backfill session persists rows. On an unbonded 5/MG - which offloads nothing and streams happily - it said the app had stored nothing from the strap while a hundred thousand HR rows sat under that very device id. Now "Offload:", with the exclusion stated outright in the zero case. And a bundle arrived on an open report with its profile started nine seconds before the export, carrying no sleep trace at all, while the capture check said "complete: all active traces present". That is what told the reporter it was ready to send. The check never looked at how long the profile had run; it does now, and says what to do rather than naming a fault. Two edges caught re-reading: a backwards clock made the profile age negative, and the offload helper was handed an age it never reads when nothing had landed. The Swift token guard was pinned against the *Trace files, which is how the bug lasted - RecoveryScorer+Trace really does write "charge term", but the engine re-emits with a day prefix before anything is logged, so the guard was pointed at the start of the pipeline and passed while the thing it guarded was dead. Its fixture carried the same flaw. Both now mirror the report. The short-profile warning is Android-only: the Swift section reports per-domain counts rather than a verdict, its renderer takes only the checks, and threading a duration through it is a signature change worth doing with that section rather than blind. Android: full suite 5,592 tests, 664 classes, 0 failures.
The Sleep hero titled its newest night "Last night" however old that night was. Both platforms measured the calendar distance from the newest RECORDED night, so at offset 0 the shown night and the reference were the same night and the answer was always zero. In the field that meant a reporter whose newest night was Saturday opened the app on Monday and saw "Last night" printed directly above "Sat 5 Sep" in accent colour: two adjacent labels contradicting each other. They read it as bad processing, and it sent the investigation into the sleep stager. The stager question is real and still open. This line was simply naming the wrong night, and the date beside it was right all along. Now counted from today, and only TODAY is rolled to the logical day. The shown night keeps the calendar wake-date the carousel groups by, because rolling that side too let two distinct entries collapse onto one label: a night ending 07:00 and the next ending 02:00 are separate entries but the same logical day. Rolling only today is the half that matters, since at 02:00 the night that ended yesterday morning is still "Last night". A NEGATIVE distance is normal here rather than the clock-skew guard it resembles: between waking before 04:00 and the roll, the night's calendar date is already tomorrow relative to the logical day, and falling back to the offset is the right answer. That branch carries a real case and is named so a later tightening cannot break it silently. The Swift logic moves to StrandAnalytics as SleepNightLabel, taking wake timestamps rather than sessions. It had lived private inside the view with no tests, which is why this defect went uncaught on that platform; this package's tests run in ordinary CI while the app-target bundle only runs on demand. The relative WORDING deliberately stays in the view, because it returns a LocalizedStringKey and moving those literals would drop them from the catalogue. The fixed 04:00 stays, deliberately. WHOOP anchors its Physiological Cycle to sleep and wake rather than to a clock, which is the better model, but a wake-anchored day needs to know when you woke - and the report behind this is a case where staging produced nothing for two nights running, so there would have been no anchor at all. Worth its own issue. Three existing Kotlin tests are repinned rather than deleted: each now states what day it is, because a count measured from today would otherwise assert against the machine's clock. Android: 5,587 tests, 664 classes, 0 failures. Swift: SleepNightLabelTests runs in test (StrandAnalytics).
CoreBluetooth exposes no link-encryption or bond state, so Apple cannot verify that a CLIENT_HELLO write completion proves an encrypted bond. Android checks the OS bond state; Apple has nothing to supply for that check, and until now the gap lived only in a source comment. A completion faster than one BLE connection interval did not come from the strap. That is exactly the signature of the false bond in ryanbr#1635, where a DISABLE_ALARM completion arriving in 5ms was misread as a hello ack, and it would have been visible on Apple with this line in place instead of only on Android. Diagnostic only. The bond still proceeds, the handshake still runs and encryptedBond is still set, because on Apple there is no alternative source of truth and refusing to bond would break every strap that genuinely bonds. The line says only what is unknown. The threshold is byte-identical to the Android constant (8), pinned by a test so a future drift fails rather than diverges quietly, and the elapsed time is captured before the hello window is consumed - read it after and the tell is always nil. Apple-only by design. Android's helloAckedWithoutEncryptionLine covers the case where the OS bond state IS available and disagrees, which is a stronger signal than a timing heuristic. Two lines for two different amounts of knowledge. The diff is purely additive against main. Author: evoveotech
… refusal (ryanbr#1804) The probe concluded that a WHOOP 5/MG refuses the offload unbonded and retired itself permanently. The evidence did not support it: all three attempts ended with status=22, GATT_CONN_TERMINATE_LOCAL_HOST, so our own stack ended the link. The probe was counting our teardown as the strap's answer, charging it to the silence budget, and latching after three. A local teardown no longer charges the silence budget. Only a strap-side drop does. But inconclusive cannot mean unbounded, and that is the harder half. On the strap this was written for, EVERY attempt was a local teardown, so simply not counting them would have left the probe re-running on every reconnect forever, on precisely the device the fix was for, with no path to a conclusion. So a local teardown charges its own budget with its own larger cap: six against the silence budget's three, because a silent link at least proved the subscriptions were accepted while a teardown proved nothing about the strap at all. Weaker evidence, more of it required, still bounded. The new budget is persisted per strap, cleared by a genuine answer, and cleared by toggling the experiment off and on - that last one matters, because the retirement line tells the user to do exactly that, and clearing only the silence budget would have made following that advice a dead end. The retirement line now names which budget ran out. A reader of a capture can tell "the strap refused", "the strap stayed silent" and "we never managed to ask" apart, which the original could not. Six new unit tests over the classification and the inconclusive line. Author: evoveotech
…anbr#1946) The Night detail grid took each tile's `latest` from Baselines.freshestCarried and threw away the day it came from. When the latest night had no data for a metric, a prior day's value appeared with only a "vs typical" caption, under a header naming tonight. A weeks-old respiratory reading could sit there indefinitely and read as tonight's. Metric now carries `latestDay` beside the value, nil when the value is today's own, so a tile that is genuinely current never stamps and the stamp keeps its meaning. Tiles prefer "Carried · <date>" over "vs typical" when the value came from a prior day, in the same "d MMM" format the Today carry stamp uses, so a carried Rest on Today and a carried metric on Sleep read identically. Both platforms, with the nilling rule expressed the same way on each, and regression tests on both sides. The caption is properly localised rather than merely extracted, which is the part CI cannot check. Android returns a DisplayText.Resource so the string lives in strings.xml where the audit and the locale files can see it, matching TodayScoring.carriedCaption; the key is present in all seven Android locales. iOS carries `Carried · %@` in all nine catalogue locales, every one translated. The seven shared locales agree word for word across the two platforms. Author: evoveotech
…ting them win the floor (ryanbr#1943) sessionRestingHR took the minimum of 5-minute bin means unconditionally, so any non-empty bin could win the night's resting HR: a one-sample bin at the edge of a wear gap, or a bin whose mean was a dropout-driven sub-physiological dip. That number is displayed, stored on the daily row, and folded into the baseline later nights are scored against. The now-removed RecoveryScorer.restingHR had exactly this hardening. The shipped path never did. A bin now qualifies to win only with at least five samples and a mean of at least 25 bpm. When nothing qualifies the floor falls back to the lowest of all bin means, then to the all-sample mean, so a sparse but genuine night keeps a floor rather than blanking. The fallback chain matters more than the thresholds. The thresholds are one constant per platform, read by both the gate and its conformance check, so a drift between the two is structurally impossible rather than merely documented. The rhrBinGateLogLine diagnostic reports the UNGATED floor against the shipped one and fires only when the gate moved it. Applied the other way round it would have gone permanently silent the moment the gate shipped, retiring the instrument before it had ever reported on a real night. Now the gate and the measurement of its effect land together, and the frequency question that ryanbr#1943 was opened to answer stays answerable. Expect the first re-score to move historical resting HR on any of the last 21 nights whose floor was won by a thin or implausible bin. That is a correction rather than a regression, and the diagnostic is what makes it legible. Both platforms, with the constants, the fallback chain and the inversion matching on each. Regression tests on both sides. Author: evoveotech
…ss complete upsertSleepSessions took whatever stagesJSON a caller passed, unconditionally, whenever the stored row was not userEdited. A device can serve the same night's hypnogram more than once - a reconnect mid-day, a resumed drain - and nothing compared the new payload against what was already there. Reproduced from a real .noopbak pair. Same primary key, same startTs: a 96-segment full night replaced in place by a 5-segment tail covering about a tenth of its own claimed span, with efficiency recomputed off just that slice and so reading 99%. No second row, no userEdited interaction. The surviving segments were correctly decoded; they were simply the wrong ten percent. Before the write, the candidate's stage-timeline richness is compared against the stored row's on the same 0/1/2 scale SleepMerge already uses to pick a day's best session for display. A strictly less complete candidate is dropped WHOLE rather than partially applied, so the row never ends up with old stages beside a new efficiency, which would be worse than either input. Equal richness still writes, so an ordinary refresh updates in place and a row cannot freeze. A userEdited row is untouched either way: the guard engages only when both sides are non-edited, and a hand-correction still wins from either direction. It lives in the store rather than in a caller deliberately. A guard in one caller protects one caller, and there are several on each platform. One consequence worth knowing: a non-userEdited row's stagesJSON can no longer be reduced or cleared, so a night staged wrongly but completely is sticky until a user edit or a delete. Richness is the only proxy available and more stages is almost always the better night, but it is a real trade rather than a free win. Android is untouched and needs a different shape, since Room's @upsert has no per-field CASE to extend. Tracked, along with the wider finding that Android has no store-level userEdited preservation either and relies on each caller to filter edited sessions. Author: pipiche38
…nsumer The frame verifiers computed a combined integrity result (header checksum, payload CRC32, structural length) but the parsers discarded it and returned a constant ok=true; downstream gates only rejected a proven-bad payload CRC, so bad-header and unverifiable frames drove live state and history metadata. ParsedFrame now carries the verifier's full verdict plus a non-optional rejection reason on both platforms; family minimum and exact lengths are enforced in the verifier; inner field reads are bounded by the CRC trailer; the six state-driving gates (router, historical classifier, stream/history extraction, clock correlation, data-range reply, offload) require the full verdict; evidence-preserving readers keep archiving rejected frames; a shared Swift-generated oracle pins verdict, reason and history classification on both sides; the Python capture tools apply the same family minimums and filter on the full verdict; docs updated. Fixes #5.
Upstream CI (macOS, xcodebuild test) rejected three synchronous test methods in BackfillMetaForgeryTests that call the @mainactor Backfiller.endData(from:family:) from a nonisolated context. Mark them @mainactor like the rest of the file; no product code changes. Post-archive fix, evidence bound to the CI rerun. (cherry picked from commit 2599c6988cfae2decf25266f6d00cee02565e805)
Owner
Author
|
CI run complete: App build (macOS + iOS) green including |
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
10 participants
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.
Fork-internal CI run only — do not merge. Same branch as upstream ryanbr#1985 (fork issue #5).
Purpose: the upstream PR is a draft because eight acceptance items need a macOS/Xcode box: both app targets compile (
Strand,NOOPiOs) and the seven new/updatedStrandTests(real recorded frames stay valid; router rejects broken-header, under-length and truncated frames; history metadata cannot be forged across offload and ack; clock correlation takes no invalid frame as anchor; the data-range reply does not narrow the offload window unchecked). This PR triggersApp build (macOS + iOS)(build +xcodebuild testforStrand),Swift Packages CI,Android CI,Source Hygiene,i18n CoverageandTools Python CIon this fork.The ninth item — one live session and one history offload per device family on a real strap, with the abort criterion from the upstream PR — remains a manual run.