fix(rhr): report resting HR as the deep-sleep mean, not the night's lowest 5-min bin

[Iskrata]

What this PR does

A night's resting HR was the minimum of its 5-minute bin means. That is wrong on NOOP's own terms, before any comparison with another device:

• The statistic did not measure what it claimed. The lowest 5-minute bin is the night's single calmest stretch, not a resting level.
• The baseline mixed two incompatible statistics. A wearer with imported history carries a baseline built from the export's resting HR, while every computed night contributes a lowest-bin figure roughly 8 bpm lower. Their own computed nights therefore read as unusually low and inflate Charge. The same low value also reaches the displayed RHR, the daily row, Apple Health and Effort's heart-rate reserve.

The session's resting HR is now the mean HR across its deep-sleep segments (SleepStager.sessionDeepSleepRestingHR). That is the slow-wave window NOOP's HRV already uses (#141). Details:

• Samples under 25 bpm are excluded. A session with fewer than 300 deep-sleep samples (~5 min) falls back to the lower quartile of the qualifying 5-min bins.
• The lowest-bin statistic stays in place for the daytime false-sleep guard, whose resting-HR dip thresholds were tuned against it. The binning code is shared between the two, not copied.
• Stored nights move with it. A one-time full-history re-score reuses the Effort re-score pass under its own flag. It waits for a pass that already holds the lock instead of giving up until the next launch (on a busy install a post-offload pass holds it at every launch). On iOS, the Apple Health write-back then deletes the resting HR it wrote since the first computed night (our own HKSource only) and writes the recomputed values.
• If that rewrite is interrupted: the owed flag is cleared only after the delete and the rewrite both return without throwing. A kill between them, or a locked phone (protected data unavailable), leaves the flag set, so the next write-back deletes again (a no-op) and rewrites. At worst there is a window in which Health has no NOOP resting HR for older nights. Duplicates can't occur, and other sources' samples are never touched.
• Strap log: the line now carries rhr= (reported) beside floor= (lowest bin) and nightMean=. The sessionRestingHR: any bin can win the night's floor, including a one-sample one #1943 bin-gate check now compares against the floor it actually tests; otherwise it would have reported a mismatch every night.

Evidence

One wearer, a WHOOP 5.0 on an iPhone 16, with raw HR read from the device DB over 23 nights. The comparison with that wearer's WHOOP history is corroboration that the new statistic lands somewhere plausible, not the argument for the change. A second wearer's numbers would help size the effect for everyone else.

Statistic	Mean over 23 nights	Night-to-night SD
WHOOP app, same wearer, last 60 days before switching	56.9 (August alone: 55.5)	3.6
Lowest 5-min bin (old)	48.9	2.2
Deep-sleep mean (new)	54.9	3.1
Whole-night mean	58.1	2.2
Last deep run only	54.2	4.4

Type of change

• Bug fix

How it was tested

• swift test in Packages/StrandAnalytics: 2025 tests pass, including the new SleepStagerDeepSleepRestingHRTests (deep mean beats a lower dip, dropouts excluded, short/absent deep-sleep fallback, empty session).
• StrandTests/IntelligenceRhrFloorMeanTests (updated line format): passes. NOOPiOS builds.
• StrandTests/IntelligenceOneShotRescoreTests: the one-time re-score waits for a pass holding the lock, then runs its own and marks itself done.
• On device: installed on the iPhone above. Nights re-scored by the normal pass show RHR in the mid-50s in NOOP and Apple Health. The one-time full-history re-score did not run on the first build (it lost the lock race at every launch), which is what the wait above fixes.
• Parity-governance suite and parity_ratchet.py --offline are clean (Python 3.12, as CI uses; 3.14 misreports the base authority); doc_comment_lint.py is clean.
• Android: the Kotlin twin (SleepStager.kt, AnalyticsEngine.kt, IntelligenceEngine.kt, the one-time re-score in AppViewModel, and tests) was built and tested by the maintainer (6235 tests, 0 failures).
  • Health Connect rewrite, added here: when the resting-HR rescore completes it sets NoopPrefs.KEY_HC_RESTING_HR_REWRITE_OWED. The next export then covers every computed day instead of its 60-day window (HealthConnectWriter.dailyCutoff). Records are upserted by the same noop-rhr-<day> clientRecordId with a higher version, so no delete is needed. The flag is cleared only after the daily insert succeeds, so a failed or interrupted export retries.
  • The lock wait is Swift-only on purpose: Android runs every pass under analyzeGate (a Mutex), so its one-time rescore waits for a running pass instead of losing the race.
  • Built and tested locally: HealthConnectRestingHrRewriteTest is new; SleepStagerDeepSleepRestingHRTest and IntelligenceRhrFloorMeanTest pass. The full suite has 6290 tests with 2 failures, both in RecoveryDriversTest (a half-tie rounding assertion). They fail identically on unmodified main on this Mac, so they are unrelated.

Checklist

• Swift package tests pass for any package I touched (swift test in Packages/StrandAnalytics)
• Android unit tests pass if I touched android/ (run locally; see the two unrelated RecoveryDriversTest failures above)
• No new build warnings introduced
• Follows the conventions in docs/CONTRIBUTING.md; FAQ and ANALYTICS.md updated
• I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

Related issues

Refs #691, #1943, #141

[ryanbr]

I can close the Android gap you flagged, and I have one substantive comment on how the change is argued.

Android: compiles and passes

Built your branch at 4ae51cc31 locally:

compileFullDebugKotlin  EXIT=0
testFullDebugUnitTest   EXIT=0
RESULT classes=731 tests=6235 failures=0 errors=0

Both new classes ran rather than silently not existing, checked by name in the result XML, and the XML timestamp was verified against the clock so this is not a stale read:

SleepStagerDeepSleepRestingHRTest  tests=4 fail=0
    theRestingHrIsTheDeepSleepMeanNotTheLowestBin
    implausibleSamplesDoNotPullTheDeepSleepMeanDown
    tooLittleDeepSleepFallsBackToTheLowerQuartileBin
    aSessionWithNoSamplesHasNoRestingHr
IntelligenceRhrFloorMeanTest       tests=4 fail=0

So the Kotlin twin is sound as written. Room schema snapshot was synced in its own invocation first, which this suite needs after a real recompile.

The argument could be stronger than the one you made

The PR leads on the WHOOP comparison, and docs/SCOPE.md is explicit that this alone will not carry a change:

A "WHOOP has it" argument, on its own, is not a reason: NOOP is a clean-room, offline, anonymous tool, not a WHOOP clone.

I do not think that sinks the PR, because the real argument is already in your description and is an INTERNAL one:

• The statistic did not measure what it claimed. The minimum of the 5-minute bin means is the night's single calmest stretch, not a resting level. That is true with no reference to WHOOP at all.
• The baseline mixed two incompatible statistics. A wearer with imported history carries a baseline built from WHOOP's resting HR while every computed night contributes a lowest-bin figure roughly 8 bpm lower, so their own computed nights read as unusually low and inflate Charge. That is NOOP being inconsistent with itself, which is a bug on any reading.

Leading with those two, and keeping the table as corroboration that the new statistic lands in a plausible place, would make this much harder to argue with. As written, a reviewer applying SCOPE.md literally could bounce it on the framing alone.

What I would still want

The evidence is one wearer over 23 nights. That is a better evidence base than most changes here get, and the direction is convincing. But this moves displayed RHR, the daily row, Apple Health, Effort's heart-rate reserve and the recovery baseline for EVERY user, and it deletes and rewrites previously exported Apple Health values. A second wearer, even briefly, would materially change how much that blast radius has to be taken on faith.

The Apple Health rewrite deserves its own scrutiny. Deleting our own HKSource resting HR back to the first computed night and rewriting is correct in intent, and it is also the least reversible thing in the PR. Worth stating explicitly what happens if it is interrupted partway.

The Android exporter does not get the equivalent rewrite, as you note. That leaves Android wearers with Health Connect rows still carrying the old statistic while the app shows the new one. Worth an issue so it is tracked rather than implied.

Nice piece of work, and the shared-binning detail (keeping the lowest-bin statistic for the daytime false-sleep guard whose thresholds were tuned against it, rather than repointing it) is exactly the kind of thing that usually gets missed.

[Iskrata]

Thanks for building the Android side. That settles the part I couldn't run.

Framing: agreed, and done. The description now leads with the two internal arguments: the old statistic measured the calmest 5 minutes rather than a resting level, and the baseline mixed imported resting HR with a figure roughly 8 bpm lower. The WHOOP table is kept only as corroboration.

One wearer: true, and I don't have a second one. The description now says so plainly, so the blast radius isn't taken on faith. Numbers from anyone with a strap-only history would be very welcome.

Interrupted Apple Health rewrite: the owed flag is cleared only after rewriteRestingHR returns without throwing, inside the same attempt:

• Killed after the delete but before the write: the flag is still set, so the next write-back deletes again (a no-op) and rewrites.
• Phone locked (protected data unavailable): the write throws, and the same retry happens.
• The delete itself failed: writeVitals still replaces every sample that carries a current key.

The worst case is a window in which Health has no NOOP resting HR for older nights. Duplicates can't occur, and other sources' samples are never touched, because the delete is scoped to HKSource.default(). This is in the description now too.

A bug in my own change: on my phone the one-time re-score never ran. At every launch a post-offload pass already held computing, so runEffortRescoreIfNeeded returned without running and retried only at the next launch, and a busy install re-arms those passes back to back. 43317e3 fixes it: it now waits for the running pass and marks itself done only when a pass at least that wide completes. IntelligenceOneShotRescoreTests covers it. That also corrects the earlier "on device" line in the description: the recent nights were right, but the full-history pass hadn't actually run.

Rebased onto current main (the branch point was 12 commits behind). No conflicts, and parity_ratchet.py --offline reports 0 errors.

Android: I'd rather close this gap than leave it implied, so I'm setting up a local Android toolchain and will add the Health Connect equivalent of the rewrite to this PR, built and tested locally before I push. I'll also check whether Android's one-time re-score can lose the same race; its passes are serialised differently, so it may not. The Kotlin twin already here stays as you verified it.

[Iskrata]

Added the Health Connect side as promised (2dcebe4). After the resting-HR rescore, the next export reaches past its 60-day window once and upserts every computed day's resting HR under the same noop-rhr-<day> id. The flag clears only after the daily insert succeeds. It was built and tested locally, and the description has the details. On the one-time rescore race: Android serialises every pass under analyzeGate, so its one-shot waits rather than losing the race, and the lock-wait commit stays Swift-only. I also rebased onto current main, which resolves the conflict.

[ryanbr]

Reviewed at 9d1d583. This is the best-argued PR in the queue right now, so let me start with what I checked that could have gone wrong, then the three things worth acting on.

Verified clean:

• The binning really is shared, not copied. restingBinMeans is extracted and both callers use it, and sessionRestingHR keeps its exact prior behaviour (gated.min then all.min then sampleMean) for the daytime false-sleep guard whose thresholds were tuned against it. Retaining the old statistic for the caller that was calibrated on it, rather than repointing everything at the new one, is the right call.
• The Health deletion is properly scoped. HKQuery.predicateForObjects(from: HKSource.default()) means a WHOOP or Apple Watch resting HR can never be touched.
• No rounding divergence. Kotlin's roundToInt() is half-up and Swift's .rounded() is half-away-from-zero, which differ only on negatives, and every value here (bin means, BPM means) is positive. Worth stating plainly because the identical construct is NOT safe in fix(import): store WHOOP's absolute skin temperature as skinTempC, not as the deviation #2285, where the value is a signed deviation.
• CI is 19 of 19 green, and the new tests are real ones: deep mean beating a lower dip, dropouts excluded, the short and absent deep-sleep fallbacks, empty session.

1. A diagnostic now reports the old statistic under the new name

SleepStager.swift:1602, the accepted-run trace, still prints restingHR=\(resting ?? -1), and resting is the floor, correctly retained for the daytime guard. The session appended two lines above now carries sessionDeepSleepRestingHR. So the gate trace reads restingHR=48 for a night NOOP reports as 55.

What makes this an oversight rather than a decision is that you fixed exactly this hazard on the other line, changing rhr day=... floor=... into rhr day=... rhr=<reported> floor=<lowest bin> nightMean=.... The principle got applied once and missed once. Note the dropped/daytime-guard trace just above it is correct to print resting, since that is genuinely what the guard tested, so the fix is narrow: it is only the verdict: .kept, gate: "accepted" line.

2. The flag-clearing claim is stronger than the code

The description says the owed flag "is cleared only after the delete and the rewrite both return without throwing". The delete is _ = try? await store.deleteObjects(...), which cannot throw. Same for the authorizationStatus == .sharingAuthorized gate: unauthorised means no delete happens at all. Either way writeVitals runs, the flag clears, and it never comes back.

Narrow, but real: the broad delete's stated purpose includes clearing samples "written under an older key scheme", which writeVitals's own key-scoped delete will not reach. That cleanup can silently not happen, permanently, and nothing records that it did not.

3. This substantially overlaps #2358

This changes AnalyticsEngine.swift:519 so a session's filled restingHR is the deep-sleep mean. #2358 changes restingHRDaily at :699 to a primary-session mean. Both target the same complaint, that the daily resting HR reads too low, from different layers. If this lands, restingHRDaily = ...restingHR.min() becomes the minimum across deep-sleep means, which absorbs most of the gap #2358 was measured against. Those MAE figures were computed against the OLD floor statistic, so they would not survive this landing unchanged.

I have said the same on #2358. The two should not be evaluated independently, and I would like a decision on sequencing before either merges.

On the evidence

I held #2358 partly because its evidence is a single participant, and this is also one wearer over 23 nights, so let me be explicit that the distinction is real rather than convenient. #2358's argument IS the comparison, so one participant undermines it directly. This PR's argument is internal: the baseline mixes the imported export's resting HR with computed lowest-bin values roughly 8 bpm lower, so a wearer's own nights read as unusually low and inflate Charge. That is a defect provable with no external reference at all, and you label the 23-night table as corroboration rather than the case. That discipline is what makes this reviewable.

Housekeeping

The branch is dirty, 27 commits behind, and carries a parity_twin_map.json stamp derived on the old base. Main's authority was migrated earlier today in f8ada93f6, so a rebase plus a plain --refresh-derived will re-derive cleanly now with no --migrate-authority needed. It would not have worked an hour ago, which is worth knowing if you tried and saw the base-authority error.

Your note that Python 3.14 misreports the base authority while 3.12 does not is worth pinning down on its own; CI pins 3.12, so nothing is broken today, but a version-sensitive derivation is a trap for the next person.

Approving once the trace label is corrected and the branch is rebased. Point 2 I would take either here or as a follow-up. Point 3 is not yours to resolve.

[ryanbr]

Sequencing decided: this goes in first, ahead of #2358.

The reasoning is the one in my review. Your case is internal to the codebase, that the baseline mixes the imported export's resting HR with computed lowest-bin values roughly 8 bpm lower, so a wearer's own nights read as unusually low and inflate Charge. That is provable here with no external reference, and it does not depend on the 23-night table being representative. #2358's argument is the comparison itself, so it has to be re-derived once the statistic underneath it moves, which is exactly what this changes.

Three things and it can go in:

1. The accepted-run trace at SleepStager.swift:1602 still prints restingHR=\(resting ?? -1), and resting is the floor. The session appended two lines above now carries sessionDeepSleepRestingHR, so a night NOOP reports as 55 traces as 48. Only the verdict: .kept, gate: "accepted" line needs it; the dropped/daytime-guard trace above is correct to print the floor, since that is genuinely what the guard tested.

2. A line in the description saying the learned habits shift. sleepConsistency, habitualMidsleepSec and sleepNeedHours move slightly for everyone on upgrade, because the most recently finished night is held out for up to a day. I think that is right and your doc comments explain the mechanism, but the PR is typed as performance and battery, and three user-visible numbers moving deserves to be on the record rather than discovered.

3. A rebase. Main's parity authority was migrated earlier today in f8ada93f6, so a plain --refresh-derived now re-derives cleanly with no --migrate-authority. That would not have worked this morning, so if you tried and hit a base-authority error, that was main and not your branch.

Point 2 on the flag-clearing claim I raised is fine as a follow-up; it does not need to hold this up.

[Iskrata]

Done, rebased onto main (ec94cb9a2):

1. Trace label. The accepted-run line now prints restingHR=<reported deep-sleep mean> floor=<lowest bin>, named like the rhr day= line, on both platforms. The dropped/daytime-guard trace still prints the floor, since that's what the guard tested. Nothing parses the field; I checked for readers before changing it.
2. Flag clearing. I took it here rather than as a follow-up, since it was small. The broad delete now try awaits, so a failed delete propagates and the owed flag stays set until a write-back succeeds. HKError.errorNoData (nothing of ours in the span) counts as done. Without resting-HR sharing permission there's nothing this app may delete, so only the write runs; the doc comment now says so. The description's claim is true now.
3. Rebase. One real conflict: fix(health): hold a still-open night out of Apple Health until it closes #2294 added holdingDays: to writeVitals. rewriteRestingHR now takes it and passes it through, so an open night stays held during the one-time rewrite too.

Locally: StrandAnalytics SleepStager|SleepReadout (182), IntelligenceOneShotRescoreTests and IntelligenceRhrFloorMeanTests, Kotlin IntelligenceRhrFloorMeanTest, SleepStagerDeepSleepRestingHRTest, HealthConnectRestingHrRewriteTest and SleepReadoutTest all pass, and NOOPiOS builds.

Point 2 in your 07:38 comment, the learned-habits note, reads as if it's about #2293 (finishedBefore). I've added it to that description.