Repository navigation
Gate watch recovery on a usable RHR baseline and valid HRV nights - #1982
Merged
Merged
Conversation
…HR baseline WatchRecovery.compute scored the RHR term whenever today's resting HR was present, even when the RHR history was empty or entirely implausible — the term then ran against Baselines.foldHistory's synthetic midpoint baseline (75 bpm), which is nobody's resting HR. The term now requires a usable RHR baseline as well, matching the `usable ? state : nil` gate the strap Charge surfaces (TodayView / CoupledView) already apply; without one the documented HRV-only path is taken, exactly as when today's reading is missing. The week gate counted raw history entries, so seven readings of which only four were physiologically valid cleared minBaselineNights. It now counts the nights the baseline actually accepted (BaselineState.nValid). Swift and Kotlin twins changed together; paired empty / unusable / usable RHR-history tests and 6-raw, 7-raw-with-3/4-valid and 9-raw-with-7-valid boundary tests on both platforms. The Kotlin expected score literal comes from a Swift oracle executable linked against the StrandAnalytics package. fork issue #61 and #62
Follow-up to the fork issue #61 and #62 fix, no product-logic change. testEmptyRHRHistoryScoresLikeMissingRHR now also asserts the absolute score the Swift oracle produced (57.932425214874954), the same literal the Kotlin twin pins, so the oracle guards both directions instead of only catching Kotlin drift. The Kotlin comment beside the rhrBaseline gate claimed the Android Charge surface applies the same `usable ? state : null` gate; ui/TodayScoring.kt passes the baseline ungated, so the comment now names only the macOS Charge driver breakdown it actually mirrors. fork issue #61 and #62
Owner
|
Reviewed, and this is an unusually careful PR. Thank you @bhelm, particularly for stating the behaviour change in numbers and for flagging the wider scope yourself rather than letting me find it. I checked the load-bearing claims against the tree rather than taking them from the description, and they all hold:
Parity. The Kotlin is an exact mirror of the Swift, and you added exactly 7 tests per side, 1:1 paired. The file totals differ (16 Swift vs 12 Kotlin) but that gap is entirely pre-existing (9 vs 5 before this PR), so it is not yours. What I ran. Android full suite on your branch: 665 classes, 5625 tests, 0 failures, with One thing to settle before this mergesThe no-migration behaviour is described accurately, but I want to name the user-visible shape of it, because I think it is bigger than "history will hold a mix". The write-back skips nil ( A user seeing that will read it as "the update broke my Charge", not "the old numbers were wrong". So I think this needs one of:
Option 2 is enough to merge on, and it is close to the release note you already suggested. I am not asking you to build option 1 in this PR: the scope discipline here is good and I would rather keep it. But I would like the follow-up issue filed so it is not lost, alongside the Nothing else blocking from me. |
This was referenced Sep 8, 2026
Open
Android: TodayScoring passes the RHR baseline ungated where the iOS surfaces gate it on usable
#1988
Closed
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
`Baselines.foldHistory` returns the config's SYNTHETIC midpoint, about 75 bpm for
resting HR, when the history is empty or entirely out of physiological range.
That is nobody's resting HR, and every engine passed it straight through: the
headline Charge, on both platforms, was z-scored against a baseline the user had
never had.
Gated in the three places that read this baseline, rather than at each of the
eight call sites:
* `RecoveryScorer.recovery`'s BaselineState overload, which AnalyticsEngine and
IntelligenceEngine reach on both platforms. Mirrors how hrvBaselineUsable is
already derived one line below.
* `chargeDrivers`, which builds the ROW from the baseline directly, its z, its
mean and its verdict, not only through the scorer. Gating the scorer alone
would have emitted an RHR row scored against the midpoint while the headline
excluded it, which is worse than the original defect and would break the
contract chargeDrivers documents: that its rows are scored against the
identical inputs as the headline number.
* `RecoveryScorerTrace`, for the same reason again: it builds its own
`charge baseline rhr` line, its rhrZ and the saturation guard from the
baseline directly. Ungated, the trace would have named an rhr term the score
did not use, which is the one thing a trace must never do, and its own
comment promises it cannot.
This is the engine-level follow-through from #1982, which established the same
rule for WatchRecovery. `usable` is provisional-or-trusted, so a real personal
baseline that has gone stale is dropped too.
USER-VISIBLE: for a user with fewer than four valid resting-HR nights, or an
all-implausible history, or a baseline stale beyond 14 nights, the RHR term now
drops and Charge renormalises onto the remaining terms. The number moves, and it
moves for existing users on both platforms. No stored value is rewritten, so
history holds a mix of old-rule and new-rule days. Needs a release note.
Eight mirrored tests per platform, including controls proving the gate is not a
blanket off, one pinning the score and the driver rows against each other, and
two pinning the trace. The pre-existing RecoveryScorerTraceTest still passes
unchanged.
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
`Baselines.foldHistory` returns the config's SYNTHETIC midpoint, about 75 bpm for
resting HR, when the history is empty or entirely out of physiological range.
That is nobody's resting HR, and every engine passed it straight through: the
headline Charge, on both platforms, was z-scored against a baseline the user had
never had.
Gated in the three places that read this baseline, rather than at each of the
eight call sites:
* `RecoveryScorer.recovery`'s BaselineState overload, which AnalyticsEngine and
IntelligenceEngine reach on both platforms. Mirrors how hrvBaselineUsable is
already derived one line below.
* `chargeDrivers`, which builds the ROW from the baseline directly, its z, its
mean and its verdict, not only through the scorer. Gating the scorer alone
would have emitted an RHR row scored against the midpoint while the headline
excluded it, which is worse than the original defect and would break the
contract chargeDrivers documents: that its rows are scored against the
identical inputs as the headline number.
* `RecoveryScorerTrace`, for the same reason again: it builds its own
`charge baseline rhr` line, its rhrZ and the saturation guard from the
baseline directly. Ungated, the trace would have named an rhr term the score
did not use, which is the one thing a trace must never do, and its own
comment promises it cannot.
This is the engine-level follow-through from #1982, which established the same
rule for WatchRecovery. `usable` is provisional-or-trusted, so a real personal
baseline that has gone stale is dropped too.
USER-VISIBLE: for a user with fewer than four valid resting-HR nights, or an
all-implausible history, or a baseline stale beyond 14 nights, the RHR term now
drops and Charge renormalises onto the remaining terms. The number moves, and it
moves for existing users on both platforms. No stored value is rewritten, so
history holds a mix of old-rule and new-rule days. Needs a release note.
Eight mirrored tests per platform, including controls proving the gate is not a
blanket off, one pinning the score and the driver rows against each other, and
two pinning the trace. The pre-existing RecoveryScorerTraceTest still passes
unchanged.
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
`Baselines.foldHistory` returns the config's SYNTHETIC midpoint, about 75 bpm for
resting HR, when the history is empty or entirely out of physiological range.
That is nobody's resting HR, and every engine passed it straight through: the
headline Charge, on both platforms, was z-scored against a baseline the user had
never had.
Gated in the three places that read this baseline, rather than at each of the
eight call sites:
* `RecoveryScorer.recovery`'s BaselineState overload, which AnalyticsEngine and
IntelligenceEngine reach on both platforms. Mirrors how hrvBaselineUsable is
already derived one line below.
* `chargeDrivers`, which builds the ROW from the baseline directly, its z, its
mean and its verdict, not only through the scorer. Gating the scorer alone
would have emitted an RHR row scored against the midpoint while the headline
excluded it, which is worse than the original defect and would break the
contract chargeDrivers documents: that its rows are scored against the
identical inputs as the headline number.
* `RecoveryScorerTrace`, for the same reason again: it builds its own
`charge baseline rhr` line, its rhrZ and the saturation guard from the
baseline directly. Ungated, the trace would have named an rhr term the score
did not use, which is the one thing a trace must never do, and its own
comment promises it cannot.
This is the engine-level follow-through from #1982, which established the same
rule for WatchRecovery. `usable` is provisional-or-trusted, so a real personal
baseline that has gone stale is dropped too.
USER-VISIBLE: for a user with fewer than four valid resting-HR nights, or an
all-implausible history, or a baseline stale beyond 14 nights, the RHR term now
drops and Charge renormalises onto the remaining terms. The number moves, and it
moves for existing users on both platforms. No stored value is rewritten, so
history holds a mix of old-rule and new-rule days. Needs a release note.
Eight mirrored tests per platform, including controls proving the gate is not a
blanket off, one pinning the score and the driver rows against each other, and
two pinning the trace. The pre-existing RecoveryScorerTraceTest still passes
unchanged.
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.
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Sep 16, 2026
The 2026-09-09 sync moves scored VALUES, not just how they are presented, so a `dailyMetric` row computed under v2 no longer means what the current code would produce from the same inputs. Nine changes do it, each on some subset of nights: the RHR and HRV baseline gates (ryanbr#1978/ryanbr#1982/ryanbr#1990), the strain denominator's domain check (ryanbr#1977), RMSSD withheld on a night whose R-R over-counts, the ryanbr#1943 resting-HR bin gate, the closed final HR/HRV window, heart-rate recovery requiring one CONTINUOUS effort, ryanbr#1984's bridged fragmented nights, and ryanbr#2015's gravity-conditional motion re-fold. So: `Analysis migration required: yes`. `currentAnalysisRecipeVersion` 2 → 3. No new migration machinery — the existing one already has the properties CLAUDE.md asks for. `prepareAnalysisRecipe` reads the cursor, sees `.migrate(2, 3)` and runs `runAnalysisMaintenance`, which is a forced `analyzeRecent(maxDays: 21, allowDayReuse: false)` that marks the recipe cursor ONLY after the pass returns, so an interrupted or failed run re-runs rather than silently claiming the rescore. It re-derives; it deletes no raw samples and no user sleep or workout corrections. 21 nights is the narrowest interval provable rather than a round number: every one of the nine is a per-night derivation, and the engine's own scoring window is 21 nights. Older nights keep their v2 values until re-scored for another reason — the same contract v1 → v2 shipped with. Deliberately NOT in this bump: the skin-temp absolute backfill for nights OUTSIDE that window. It is its own resumable walker, fill-only on a NULL column, reached on purpose from the Test Centre rather than on launch, so folding it in would widen the migration to the whole library for a value the rescore does not compute anyway. The decision tests are extended where they were weakest: the three existing cases are written against `current`, so they stay green THROUGH a bump without ever witnessing one. Two literal pins are added — that a v2 install migrates as `2 → 3`, and that an install already at 3 (or rolled back from a 4) never rescores on relaunch, which is the failure mode using a marketing or build number here would cause and which is invisible until someone's phone spends twenty minutes re-scoring after a cosmetic update. The first pin is MEANT to fail on the next bump: that failure is the prompt to answer the migration question for whatever the bump carries. Verified: StrandTests 2950, 0 failures; all nine packages green; macOS and iOS both build; doc_comment_lint clean. Recorded in docs/fork/decisions.md. Analysis migration required: yes (this commit is it).
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.
Problem
Two defects in the same function,
WatchRecovery.compute, on both platforms.bhelm#61: the RHR term was keyed only on today's reading —
rhrBaseline: todayRHR != nil ? rhrBase : nil. With an empty or all-implausible RHR history,foldHistoryreturns the config's synthetic midpoint (≈75 bpm), which is nobody's resting HR, andCharge was z-scored against it.
bhelm#62: the week-long warm-up gate read
sdnnHistory.count >= minBaselineNights— theraw history length. Nights the baseline rejected as implausible are skip-and-held by
Baselines.updateand shape nothing, yet still bought a score a week early. Both sit in the sameguard, so they ship as one PR; splitting them would mean two conflicting edits to the same lines.
Change
Two expressions per platform (
WatchRecovery.swift,WatchRecovery.kt):hrvBase.nValid >= minBaselineNightsreplaces the rawsdnnHistory.count/hrvHistory.size.nValidis incremented only for in-range folded nights, so it is exactly "nights that shaped thebaseline".
hrvBase.usableis kept alongside it — it still excludes a stale baseline.rhrBaseline: (todayRHR != nil && rhrBase.usable) ? rhrBase : nil. Without a usable RHR baselinethe call falls into the already-documented HRV-only path, exactly as when today's reading is
missing;
RecoveryScorerdrops and renormalises the term. This mirrors the gate the macOS Chargedriver breakdown applies (
TodayView/CoupledView).Remaining lines update the doc comments on the two touched declarations and the file headers, which
stated the old gate. Mirrored tests (+80 Swift / +108 Kotlin): empty / unusable / usable RHR
history, and 6-, 7- and 9-night raw histories against 3, 4, 6 and 7 valid nights.
Verification
cd Packages/StrandAnalytics && swift test --filter WatchRecoveryTests→Executed 16 tests, with 4 failures— the empty and unusable RHR-history cases both scored98.574001231305against the expected57.932425214874954, andtestSevenRawNightsWithFourValidCalibratesscored51.12773242026989where nil was expected(
"building"vs"calibrating")../gradlew testFullDebugUnitTest --tests "com.noop.analytics.WatchRecoveryTest"→12 tests completed, 3 failed(same three cases).tests="12" failures="0" errors="0". Regression check on the other caller-level suite runnable here:--tests "com.noop.analytics.IntelligenceWatchRecoveryTest"→tests="5" failures="0".Packages/StrandAnalyticsprinted57.932425214874954for both the with-reading and no-readingcases, pasted verbatim into the Kotlin test with its body in the KDoc. It links the production
package rather than a hand-copied twin, so it cannot drift. The Swift test pins the same literal
(
accuracy: 1e-12), so the oracle holds in both directions.python3 Tools/doc_comment_lint.py→OK no NEW detached doc comments.StrandTests/IntelligenceWatchRecoveryTests.swiftand every macOS/iOS/watch apptarget — no macOS/Xcode available to me. Those fixtures use 10 fully valid days, so the gate
change does not alter them, but that is reasoning, not a run. No strap, no device, no emulator.
Behaviour change / user-visible effects
These are real, user-visible moves — worth reading before merging:
instead of one scored against the synthetic ≈75 bpm midpoint. The number can move a lot: in
the oracle fixture (7 nights at HRV 45, today's HRV 45, today's RHR 52, no RHR history) Charge
goes 98.57 → 57.93. The new value is the honest one; the old one was flattered by a baseline
the user never had.
will see a ring go back to "calibrating".
history will hold a mix of old-rule and new-rule days; only newly computed days use the new gates.
Oura / Fitbit / Garmin / Health Connect, so both effects reach those users too. Worth a release
note.
usableexcludes.stale, the RHR term is now also dropped when a real, personal RHRbaseline has had no valid night for over 14 nights — not only for the synthetic midpoint docs: fix two remaining dead NoopApp/noop links (#60 follow-up) #61
names. I believe that is correct and consistent with the codebase's use of "usable" (the word
docs: fix two remaining dead NoopApp/noop links (#60 follow-up) #61's acceptance criteria use), but flagging it so the wider scope is a conscious call.
Scope limits and follow-ups
BaselineState.usable(provisional or trusted,nValid >= minNightsSeed= 4); "minBaselineNightsnights" as accepted nights (nValid). Both are thecodebase's own definitions, not new ones. Stale-HRV behaviour is unchanged.
Baselines,RecoveryScorer,HRVReadiness, all calibration messaging,and the
rhr:argument (stillrhrBase.baselinewhen today's reading is missing — withrhrBaselinenil that value is unused, and leaving it keeps the diff minimal).TodayScoringpasses its RHRbaseline ungated where macOS
TodayViewgates it. That drift deserves its own issue.Branch:
fix/issue-61-62-watch-recovery-gates. Contribution offered under the repository'sPolyForm Noncommercial 1.0.0 license.