Skip to content

recovery: treat an unusable resting-HR baseline as absent (#1988) - #1990

Merged
ryanbr merged 1 commit into
mainfrom
fix-1988-engine-rhr-gate
Sep 8, 2026
Merged

ryanbr merged 1 commit into
mainfrom
fix-1988-engine-rhr-gate

Conversation

@ryanbr

@ryanbr ryanbr commented Sep 8, 2026 •

Copy link
Copy Markdown
Owner

Addresses #1988. Supersedes #1989, which gated only the Android driver row; see below for why that was the wrong shape.

The defect

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. Every engine passed that straight through, so the headline Charge on both platforms was z-scored against a baseline the user has never had. This is the same defect #1982 addressed for WatchRecovery, in the path that reaches the ring.

Where the gate goes, and why three places

Not at the eight call sites, but at the three places that read this baseline:

  1. RecoveryScorer.recovery's BaselineState overload. AnalyticsEngine and IntelligenceEngine reach it on both platforms. This mirrors how hrvBaselineUsable is already derived one line below, so the two baselines are now handled the same way.

  2. chargeDrivers. This one is easy to miss and it is why the first attempt was wrong: chargeDrivers 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 falsify the contract chargeDrivers documents, that its rows are scored "against the identical inputs as the headline number".

  3. RecoveryScorerTrace. Found on a re-review, and the same shape a third time: the trace builds its own charge baseline rhr line, its rhrZ and the saturation guard from the baseline directly. Ungated, it would have named an rhr term the score did not use, which is the one thing a trace must never do, and which its own comment promises cannot happen: "The score the dashboard reads, verbatim, so the trace cannot diverge from it."

Gating all three keeps that invariant true, which is the point. Gating any subset is worse than gating none: the score and the thing describing the score would disagree.

Why not #1989

#1989 gated only Android's TodayScoring, to match the iOS views. Re-reviewing it showed that would have made things worse in a specific way: Android currently satisfies the identical-inputs contract (drivers and headline both ungated, both wrong), and a view-only gate would have introduced the divergence on a platform that did not have one. iOS already has it, since its views gate while its engine does not. This PR removes the divergence on both platforms instead of spreading it.

User-visible effect, and it is real

For a user with fewer than four valid resting-HR nights, 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, for existing users, on both platforms.

No stored value is rewritten, so history will hold a mix of old-rule and new-rule days, the same situation #1987 tracks for #1982. This needs a release note. Since usable is provisional-or-trusted, a real personal baseline that has gone stale is dropped as well, which is the reading of "usable" the rest of the codebase uses.

Tests

Eight mirrored per platform, 1:1 by name. Three are controls proving the gate is not a blanket off: a usable baseline still contributes to the score, still produces its row, and still produces its trace term and baseline line. One pins the invariant directly, that passing an unusable baseline is indistinguishable from passing none, row for row.

Verified non-vacuous rather than assumed: with the gates reverted, 4 of the 6 original tests go red and every control stays green. The pre-existing RecoveryScorerTraceTest passes unchanged at 10/10, so the trace's other invariants are intact.

Full Android suite: 668 classes, 5,648 tests, 0 failures. doc_comment_lint.py clean. Swift is CI-verified as always; I checked the Swift API assumptions by hand first, which caught that ChargeDriver.label is a String there while the Kotlin twin uses a ChargeDriverLabel enum.

`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
ryanbr force-pushed the fix-1988-engine-rhr-gate branch from 8fb6ae6 to 152c4b3 Compare September 8, 2026 02:27
@ryanbr
ryanbr merged commit e7128ba into main Sep 8, 2026
16 checks passed
@ryanbr
ryanbr deleted the fix-1988-engine-rhr-gate branch September 8, 2026 02:41
ryanbr added a commit that referenced this pull request Sep 8, 2026
Three small follow-ups to #1990, no behaviour change.

RecoveryScorer.kt's `@param rhrBaseline` still described only "null drops the
RHR term" while its Swift twin was updated to name the unusable rule. That is a
doc-parity gap introduced by #1990 itself: the same parameter documented two
different ways in the two languages is the drift the parity contract exists to
prevent.

TodayScoring folds an rhr baseline and passes it ungated, directly above a resp
baseline that IS gated with takeIf. Before #1990 that asymmetry was the bug; now
it is safe only because chargeDrivers gates internally, for its score AND for the
row it builds from the baseline directly. Without a note the next reader either
adds a redundant gate or concludes the work was left half done, so the reason is
now written where the asymmetry is.

The UI wiring suite gains ONE case from the superseded PR, pinning the end-to-end
path at the surface a user sees: the RHR row is absent when the baseline is
unusable. It PASSES WITHOUT ANY PRODUCTION CHANGE, since #1990 already gates the
callee, so it is a regression pin at the UI boundary rather than evidence of a
fix. It is worth having because nothing else covers the user-visible end of that
path, and its doc names the callee that holds the gate so a reader does not hunt
for it in this file.

The usable-baseline control that PR also carried is deliberately NOT brought
over: scoredDayProducesDriverRows already uses the same fixture and already
asserts the row is present, so it added no coverage. The remaining test points at
it instead of restating it.

Full Android suite: 668 classes, 5,649 tests, 0 failures. No Swift touched.
ryanbr added a commit that referenced this pull request Sep 8, 2026
Four small follow-ups to #1990, no behaviour change.

RecoveryScorer.kt's `@param rhrBaseline` still described only "null drops the
RHR term" while its Swift twin was updated to name the unusable rule. That is a
doc-parity gap introduced by #1990 itself: the same parameter documented two
different ways in the two languages is the drift the parity contract exists to
prevent.

TodayScoring folds an rhr baseline and passes it ungated, directly above a resp
baseline that IS gated with takeIf. Before #1990 that asymmetry was the bug; now
it is safe only because chargeDrivers gates internally, for its score AND for the
row it builds from the baseline directly. Without a note the next reader either
adds a redundant gate or concludes the work was left half done, so the reason is
now written where the asymmetry is.

The UI wiring suite gains ONE case from the superseded PR, pinning the end-to-end
path at the surface a user sees: the RHR row is absent when the baseline is
unusable. It PASSES WITHOUT ANY PRODUCTION CHANGE, since #1990 already gates the
callee, so it is a regression pin at the UI boundary rather than evidence of a
fix. It is worth having because nothing else covers the user-visible end of that
path, and its doc names the callee that holds the gate so a reader does not hunt
for it in this file.

The usable-baseline control that PR also carried is deliberately NOT brought
over: scoredDayProducesDriverRows already uses the same fixture and already
asserts the row is present, so it added no coverage. The remaining test points at
it instead of restating it.

The iOS side of that path had no test at all, because the derivation lived as a
private method inside TodayView with a byte-identical copy inside CoupledView,
reachable only by rendering the view. Both are now one `ChargeBreakdownWiring
.breakdown`, lifted out unchanged: the two copies differed only in which row they
show and where their sleep-performance number comes from, both inputs. That
removes the duplication and lets the Android suite finally have a twin, which it
now does, with the one case that has no Swift counterpart named in the file
rather than left as a silent gap.

Full Android suite: 668 classes, 5,649 tests, 0 failures, unaffected by this
change. The Swift half is CI-verified as always.
ryanbr added a commit that referenced this pull request Sep 8, 2026
…1991)

Four small follow-ups to #1990, no behaviour change.

RecoveryScorer.kt's `@param rhrBaseline` still described only "null drops the
RHR term" while its Swift twin was updated to name the unusable rule. That is a
doc-parity gap introduced by #1990 itself: the same parameter documented two
different ways in the two languages is the drift the parity contract exists to
prevent.

TodayScoring folds an rhr baseline and passes it ungated, directly above a resp
baseline that IS gated with takeIf. Before #1990 that asymmetry was the bug; now
it is safe only because chargeDrivers gates internally, for its score AND for the
row it builds from the baseline directly. Without a note the next reader either
adds a redundant gate or concludes the work was left half done, so the reason is
now written where the asymmetry is.

The UI wiring suite gains ONE case from the superseded PR, pinning the end-to-end
path at the surface a user sees: the RHR row is absent when the baseline is
unusable. It PASSES WITHOUT ANY PRODUCTION CHANGE, since #1990 already gates the
callee, so it is a regression pin at the UI boundary rather than evidence of a
fix. It is worth having because nothing else covers the user-visible end of that
path, and its doc names the callee that holds the gate so a reader does not hunt
for it in this file.

The usable-baseline control that PR also carried is deliberately NOT brought
over: scoredDayProducesDriverRows already uses the same fixture and already
asserts the row is present, so it added no coverage. The remaining test points at
it instead of restating it.

The iOS side of that path had no test at all, because the derivation lived as a
private method inside TodayView with a byte-identical copy inside CoupledView,
reachable only by rendering the view. Both are now one `ChargeBreakdownWiring
.breakdown`, lifted out unchanged: the two copies differed only in which row they
show and where their sleep-performance number comes from, both inputs. That
removes the duplication and lets the Android suite finally have a twin, which it
now does, with the one case that has no Swift counterpart named in the file
rather than left as a silent gap.

Full Android suite: 668 classes, 5,649 tests, 0 failures, unaffected by this
change. The Swift half is CI-verified as always.
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant