Repository navigation
fix: only claim a held connection when the OS actually reports one - #2003
Merged
Merged
Conversation
ryanbr
force-pushed
the
fix/held-link-require-acl
branch
2 times, most recently
from
September 8, 2026 09:20
a4bca9d to
73887b3
Compare
#2002 inferred a held connection from a refused MTU exchange with no inbound traffic. That shape also describes a STALE PAIRING, and the #1997 reporter turned out to be exactly that: no official WHOOP app installed, a bond that had gone stale, and unpairing plus re-pairing is what got them connected again. So the guide would have told the one user it was written for not to re-pair, which was the thing most likely to fix them, and to quit an app they do not have. Worse advice than the guide it replaced. isStrapAclHeld already answers "is the connection still held" directly, so that is what the predicate asks. A stale pairing keeps the re-pair guide. The MTU refusal is no longer part of the test. It was how this INFERRED a held link before asking the OS directly; once the direct signal is required it adds no part of the claim and only narrows when the branch can fire, which on a branch with no field sighting yet is cost without benefit. It still reads as a refusal in the log, where it helps triage. The paired hint variant goes with it. Its branch sat after authRefusal, which takes precedence on the only path a WHOOP 4.0 reaches, and the other caller needs an unacked CLIENT_HELLO, which is 5/MG only. It could not fire for the case it was written for. The guide, which is the surface the reporter actually saw, covers it. The wording leads with turning Bluetooth off and on, which works whoever holds the connection, and mentions the official app second and conditionally. The pause also logs what the choice turned on. Nothing in the field log shows the ACL signal on the silent links, because those took the give-up path, which prints no ACL marker. So the branch may be right and may simply never fire, and without this line the next log would not say which.
ryanbr
force-pushed
the
fix/held-link-require-acl
branch
from
September 8, 2026 09:27
73887b3 to
a2f910b
Compare
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Sep 16, 2026
Thirty real conflicts. The Android tree is dropped again per FORK_GUIDE; every other hunk was resolved by asking which mechanism this fork actually runs, not by preferring a side. Taken from upstream: • Protocol: ryanbr#2017/ryanbr#2018/ryanbr#2020 — the 5/MG historical optical and raw-IMU layouts, and the v26 window's ABSOLUTE base code beside its deltas. Its migration lands as `v57-ppg-waveform-base-code` (upstream's v44), since this fork's sequence is nine ahead of theirs. • BLE diagnostics ryanbr#1998-ryanbr#2003: a bond end that says WHY, named BLE integers, a session-budgeted hex dump, and "we hold the link" only when the OS says so. • Battery: ryanbr#1951 stop asking a 5/MG for a reading it refuses; ryanbr#1959 poll 0x2A19 for an unbonded one. • Scoring: ryanbr#1978/ryanbr#1982/ryanbr#1990 the RHR/HRV baseline gates, ryanbr#1977 the strain denominator's domain, RMSSD withheld on an over-counted night, ryanbr#1943 the resting-HR bin gate, the closed final HR/HRV window, HR-recovery requiring one CONTINUOUS effort, ryanbr#1984 nights bridged under minSleepMin, ryanbr#2015 motion re-folded only when that day's gravity moved, ryanbr#2016 a provisional Rest. • Charts ryanbr#2004/ryanbr#2005/ryanbr#2007/ryanbr#2009/ryanbr#2011 and ryanbr#1995's hero-ring detail route, ryanbr#1164's "Pending sync" caption under Rest. • Widgets: the heart-rate widget with its trace (ryanbr#1957/ryanbr#1964) and the widget cost counters. • Health: ryanbr#1503 — the Apple Health external UUID drops the strap id, so a re-pair no longer strands every prior record, plus the one-off sweep that reclaims the stranded ones; ryanbr#1205 GPS routes read back from Apple Health. • Storage: rolling retention for ppgWaveformSample, the skin-temp absolute backfill outside the 21-night window, per-table byte estimates. • Units split (body vs exercise distance), per-day wake overrides, the wind-down nudge pinned to its own day, and the German Sie→du conversion (29 entries, merged per LOCALE so the fork's own de edits stayed). Kept on the fork's side, with the reason: • ryanbr#1005's in-memory `dayScanCache` stays out, again: the persisted `DayScanFingerprint` survives relaunch and its key folds the per-day weight and max HR upstream's does not. • The steps-motion cache stays PERSISTED for the same reason. Upstream's failed-read-vs-empty-day distinction IS adopted into it — on a persisted cache a stale zero from a read error would outlive the process that made it. • Every stream read in the day loop keeps `limit: Int.max`. All five are uncapped together; taking StreamReadCap for three of them would leave the loop half-capped, which is worse than either choice. • The coach engine. Upstream's K1-K15 layer is not adopted: its single 40-row `coachMessage` table, its tool-less pre-baked context and its own brief are behind what this fork already ships. Its ADDITIVE half is kept — the on-device voice input, the Siri "Ask Coach" intent, the brief scheduler and brief widget, and streaming for the OpenAI-compatible providers this fork's Anthropic-only StreamingToolClient does not cover. They are wired up in the next commit; `CoachBriefWidget` is deliberately not registered until then. • Liquid's `DomainTheme` labels, value-sampled Charge tint, `LiquidScoreGauge` and `TodayCardSurface`; classic Today's ring order and `LiveEffort.today`. • RootTabView's four-tab shell and `MoreCatalog`: Coach hangs on content here, not on a tab. Two duplicate mechanisms collapsed rather than carried: • `ChargeBreakdownWiring` (upstream) and `ChargeBreakdownFormat` (fork) are the same extraction done twice. The fork's survives, Coupled now reads it, and upstream's TEST is kept — it is coverage this path never had. • Upstream's `CoachMessageStore` is dropped; `CoachConversationStore` already autosaves the whole multi-conversation history. One defect found while resolving, fixed here: `LiveEffort.today` read HR with no explicit limit, so `hrSamples`' 8000-row default truncated it — at ~18k rows a day that covered about ten hours and the live Effort silently stopped climbing. It failed safe (`effectiveEffort` takes the max), which is why it went unseen. `Live5RestFrozenTests` is removed by upstream, which replaces it with `Live5NoGravityRestChainTests`. `local_day_windows_oracle.json` moves out of upstream's android path into `Tests/StrandAnalyticsTests/Oracles/` — NOT `Fixtures/`, which this repo's .gitignore excludes everywhere. Verified: StrandAnalytics 2233, WhoopStore 627, WhoopProtocol 700, StrandImport 293, StrandDesign 120, NoopLocalAccess 14, SemanticMemory 19, OuraProtocol 209 and PolarProtocol 25 all green; macOS and iOS both build; doc_comment_lint clean; `i18n_audit --ci` byte-identical to its pre-merge output (it is red on main for reasons that predate this, and the seven new widget literals were given all nine locales so the gate did not get worse). Six StrandTests were ALREADY failing on main before this merge and still are — `CoachMyLogsAndZonesTests` (the tool census pin was not bumped when the Hevy tools landed) and five in `RawPhysiologyUnionTests`. Every input to them — CoachTools.swift, ToolConsent.swift, Repository.swift and both test files — is byte-identical to main, so this merge neither caused nor fixed them. They went unnoticed because app-build.yml last ran on 2026-08-25. Analysis migration required: YES. The scoring changes above make stored derived values stale; the recipe bump and its resumable migration are the next commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2002, before it reaches a build.
That change inferred a held connection from a refused MTU exchange with zero inbound traffic. Working
through #1997 further showed the inference is not safe: the same shape describes a stale pairing, and
the reporter turned out to be exactly that. No official WHOOP app installed, a bond that had gone stale,
and unpairing plus re-pairing is what got them connected again.
So on their next failure #2002 would have shown them a guide saying re-pairing will not change this, when
re-pairing is the thing most likely to fix them, and telling them to quit an app that is not on their
phone. That is worse advice than the guide it replaced, for the one user it was written for.
The change
isStrapAclHeldalready answers "is the connection still held" directly, fromgetConnectedDevices. Thepredicate now requires it rather than inferring it, so a refused exchange with no traffic only reads as a
held link when the stack confirms one. A stale pairing keeps the re-pair guide it should have had.
The wording also leads with turning Bluetooth off and on, which works whoever holds the connection, and
mentions the official app second and conditionally.
Tests
The narrowing is pinned directly:
aclHeld = falsewith the same refused exchange and zero traffic doesNOT read as a held link. The guide tests now also pin the ORDER, since leading with an app the user may
not have installed is the specific mistake being corrected.
672 classes / 5696 tests, 0 failures. Android-only, as #2002 was.