fix(health-connect): workout calories under-report, and double-count across sources by ryanbr · Pull Request #842 · ryanbr/noop

ryanbr · 2026-07-26T22:02:14Z

Fixes #835 (the calories half — the Effort request is tracked separately, see below).

Two bugs in the same credit, pulling opposite ways.

1. Under-report

A workout's energyKcal came only from ActiveCaloriesBurned records overlapping its window, prorated by time. That fails two common shapes:

The source writes TotalCaloriesBurned and no Active at all. The ride gets credited with nothing but unrelated background Active that happens to overlap — typically phone step-derived burn. The importer's own comment scoped the assumption: "Garmin/Fit write ActiveCaloriesBurned as interval records." Plenty of apps don't.
The only Active cover is a coarse record. One per day is common, and prorating assumes calories burn uniformly, so a hard hour inside a 24 h record reads as 1/24 of the day. This behaviour was deliberate and tested (aDaySpanningRecordOnlyContributesTheSessionsFraction), which is exactly why it survived — and on re-review it's the likelier cause of the report than the Total-only story I first posted on the issue.

Now also derives a candidate from Total: prorated Total over the window, minus the window's share of the day's basal burn. Prorating basal is sound in a way prorating a workout's active burn is not — basal genuinely is near-uniform. Takes whichever estimate is larger, so a stray background record loses to real session coverage without needing a magic threshold, and a good Active credit is never replaced. Null when neither stream covers the session — no fabricated number.

2. Double-count

sumActiveKcalInWindow summed every overlapping record across all sources, so a phone and a watch both logging one ride roughly doubled it. Day totals already de-overlap per source (#589), and the read-site comment claimed the workout credit did too:

"...it credits workouts by time-overlap, a separate de-overlap..."

It didn't. Now sums within a source and takes the max across them, matching the day path. Comment corrected rather than left asserting the old claim.

Notes

The upgrade runs as a post-read pass because the Total-derived estimate needs the day basal (total − active), only known once every record is read. The existing HR and distance passes over workouts already work this way.
Parity: Health Connect ingestion is Android-only. I checked for a Swift twin rather than assuming — the one hit is a comment reference in SleepMerge.swift. No parity obligation here.
Tests: the six existing assertions are unchanged and still pass — proration and partial overlap are preserved deliberately, since they're right for the uniform case. Eight added covering both bugs, both directions, and the null paths. All ten verified against an independent implementation of the algorithm before commit.
Not compiled or run by CI — android.yml is disabled. The helpers are pure and fully covered, so a staging build is what actually exercises this.

What this does not fix

If a source writes only coarse daily records for both streams, the estimate is still a prorated slice. There's no data to do better with, and inventing a distribution would be a guess. @bartmuskala's export would show which shape their app writes — asked on the issue.

The Effort-instead-of-calories request needs per-sample HR import from Health Connect (the importer currently collapses HeartRateRecord to a per-day average), which is a bigger piece of work — filed separately.

…across sources (#835) Two bugs in the same credit, pulling opposite ways. UNDER-REPORT. A workout's energyKcal came only from ActiveCaloriesBurned records overlapping its window, prorated by time. That fails two common shapes: - the session's source writes TotalCaloriesBurned and no Active at all, so the ride is credited with nothing but unrelated background Active that happens to overlap; - the only Active cover is a COARSE record (one per day is common) and prorating it assumes calories burn uniformly, so a hard hour inside a 24h record reads as 1/24 of the day. That behaviour was deliberate and tested (aDaySpanningRecordOnlyContributesTheSessionsFraction), which is why it survived. Now also derives a candidate from Total: prorated Total over the window minus the window's share of the day's basal burn. Prorating BASAL is sound in a way prorating a workout's active burn is not — basal really is near-uniform. Takes whichever estimate is larger, so a stray background record loses to real session coverage without needing a magic threshold, and a good Active credit is never replaced. Returns null when neither stream covers the session: no fabricated number. DOUBLE-COUNT. sumActiveKcalInWindow summed every overlapping record across ALL sources, so a phone and a watch both logging one ride roughly doubled it. Day totals already de-overlap per source (#589) and the read-site comment claimed the workout credit did too — it did not. Now sums within a source and takes the max across them, matching the day path. Comment corrected rather than left asserting the old claim. The upgrade runs as a post-read pass because the Total-derived estimate needs the day basal (total - active), only known once every record is read. Existing passes over `workouts` (HR, distance) already work this way. Health Connect ingestion is Android-only — no Swift twin, so no parity obligation here; checked rather than assumed. Tests: the six existing assertions are unchanged and still pass (proration and partial overlap are preserved deliberately — they are right for the uniform case); eight added covering both bugs, both directions, and the null paths. All ten verified against an independent implementation of the algorithm before commit. Not compiled or run by CI: android.yml is disabled. The helpers are pure and covered.

…helper for what it does Two things found re-reviewing my own change. The construction-time energyKcal call became dead work. The post-pass takes a max that already includes the Active-only value, so it always supersedes it when non-null — and when both are null the field stays null either way. Leaving it made the import scan the record lists THREE times per workout where it used to scan once. Now two: the Active list as before, plus the Total list the fix needs. Cost documented at the call site, with the fix (sort + range-scan) noted for whoever hits it. sumActiveKcalInWindow now aggregates Total records too, so the name actively misleads. Renamed to sumKcalInWindow, including the doc reference in HealthExportPlan. No behaviour change: same inputs, same outputs, one fewer scan.

ryanbr · 2026-07-26T22:06:43Z

Re-review found two things in my own change; both pushed.

1. A performance regression I introduced. The construction-time energyKcal call became dead work — the post-pass takes a max that already includes the Active-only value, so it always supersedes it when non-null, and when both are null the field stays null anyway. Leaving it made the import scan the record lists three times per workout where it used to scan once. Now two: the Active list as before, plus the Total list the fix needs. The O(workouts × records) shape is pre-existing, but I was about to make it worse without noticing. Cost documented at the call site with the remedy (sort + range-scan) noted for whoever hits it.

2. sumActiveKcalInWindow now aggregates Total records too, so the name actively misleads. Renamed to sumKcalInWindow, including the doc reference in HealthExportPlan.

No behaviour change from either — same inputs, same outputs, one fewer scan. CI green.

On whether this actually fixes @bartmuskala's case, honestly: it covers two of the shapes that produce "too low", not all of them.

what their app writes	fixed?
Total for the session, no Active	yes
coarse daily Active + session-scoped Total	yes
coarse daily records for both streams	no — no data to do better with
proper session Active, still looks low	no — different cause

There is also a case that is not a bug at all: if the comparison number is roughly one basal-hour higher (~60–90 kcal for an hour), that is the active vs total distinction rather than an error. Most fitness apps show active for a workout; some show total. Worth knowing which before chasing it further.

So: likely fixes it, not certainly. The export would settle which row of that table they are in.

I inserted the #835 block between the HR pass's explanatory comment and its loop, so that comment introduced the calories loop and the HR loop was left undocumented. Moved above it. Each of the three workouts passes now has its own comment directly above its own loop. Comment placement only; no code change.

#846) Kotlin takes only the LAST /** */ before a declaration as its KDoc. Where two are stacked, the first silently becomes a dangling comment — invisible to Dokka and IDE hover, while still reading fine to a human scrolling past. That is why these survive review. HrvAnalyzer.classifyCoverage: the detached block is "Byte-parity twin of Swift classifyCoverage" — a parity-contract statement, and the parity rule is the first thing in CLAUDE.md. Merged into the KDoc alongside the NaN-comparison reasoning. PuffinExperiment.PREFS: two near-identical blocks, the first a strict subset of the second. Dropped. HealthConnectImporter: introduced in #842 earlier today by inserting KcalRecord between maxSourceDouble KDoc and maxSourceDouble itself, so the comment documented the record and the function lost its docs. KcalRecord lifted above the KDoc. All three came from editing by insertion rather than in place. Provably comment-only: stripping comments and string literals leaves a byte-identical ORDERED token stream against main in all three files, so the compiler sees the same program. That matters here because android.yml is disabled and nothing compiles this. A repo-wide sweep reports zero remaining stacked sites, and zero of the sibling "doc block, blank line, declaration" pattern. Two historical copies survive in docs/superpowers/plans/ and are left alone — a dated plan records what was decided then, not a spec to keep current.

Kotlin and Swift attach a doc block to the declaration that FOLLOWS it, and keep only the LAST of several stacked ones. Insert a block above an existing one, or a declaration into the gap between a comment and the thing it documents, and the text is silently orphaned — still there, still above the right function, still correct prose, but no longer reaching Dokka or IDE hover. That is why review does not catch it. #846 fixed three sites in one day, one of which #842 had introduced that same morning, and the block that came adrift was "Byte-parity twin of Swift classifyCoverage" — a parity-contract statement, and parity is the first rule in CLAUDE.md. That is the case for gating rather than only fixing. Tools/doc_comment_lint.py catches both detaching patterns across Kotlin and Swift: stacked blocks, and a blank line between a doc block and its declaration. File headers (a blank line before package/import/@file:) are exempt, since that spacing is conventional and correct. Corrects #846, which claimed the tree was clean afterwards. It was not: that sweep only matched SINGLE-LINE blocks. Handling multi-line blocks finds 25 more pre-existing sites across 13 files — 8 in WhoopBleClient.kt alone, and AppViewModel.kt:68 is a class doc detached by an inserted data class, the same shape as the #842 slip. Those 25 are grandfathered per-file so the gate starts green and blocks only NEW ones. No existing code can red-check a contributor on a line they did not touch — the #514 failure mode. Baselined by COUNT, not line number: line numbers churn on every edit above them, which keeps the file stale and trains people to regenerate it without reading it. A count moves only when someone adds or removes a site. It ratchets DOWN — fixing one prints an IMPROVED line asking for the allowance to be lowered. Verified five ways: passes on clean main; catches all three pre-#846 sites when reconstructed; fails on a new site in an unbaselined file; fails on an EXTRA site in an already-baselined file (so the count is enforced, not mere presence); and clears nine false-positive probes including file headers, a doc above an annotation, and a `*/` appearing inside prose. Pure Python on ubuntu, six seconds. No Xcode or Gradle, so unlike the app-target and Android checks it actually runs while app-build.yml and android.yml are disabled. Its own job gates itself. New workflow rather than a step in i18n-coverage.yml: different concern, and that job is currently the only thing gating PRs — a docs regression should not report itself as an i18n failure. Not included: fixing the 25. Each needs a judgement call about how to merge two blocks, and doing 25 of those at volume is how the mistake gets made. The baseline makes them visible and burnable-down without blocking anyone. Note the 25 are machine-detected; three were spot-checked as genuine, the rest are grandfathered either way.

…across sources (ryanbr#842) Two bugs in the same credit, pulling opposite ways. UNDER-REPORT. A workout's energyKcal came only from ActiveCaloriesBurned records overlapping its window, prorated by time. That fails two common shapes: the session's source writes TotalCaloriesBurned and no Active, so the ride is credited with nothing but unrelated background Active; or the only Active cover is a COARSE record (one per day is common) and prorating assumes calories burn uniformly, so a hard hour inside a 24h record reads as 1/24 of the day. The second was deliberate and tested (aDaySpanningRecordOnlyContributesTheSessionsFraction), which is why it survived, and on re-review it is the likelier cause of the report than the Total-only story. Now also derives a candidate from Total: prorated Total over the window minus the window's share of the day's basal burn. Prorating BASAL is sound in a way prorating a workout's active burn is not — basal really is near-uniform. Takes whichever estimate is larger, so a stray background record loses to real session coverage without a magic threshold, and a good Active credit is never replaced. Null when neither stream covers the session: no fabricated number. DOUBLE-COUNT. The credit summed every overlapping record across ALL sources, so a phone and a watch both logging one ride roughly doubled it. Day totals already de-overlap per source (ryanbr#589) and the read-site comment claimed the workout credit did too — it did not. Now sums within a source and takes the max across them, matching the day path. Comment corrected rather than left asserting the old claim. Runs as a post-read pass because the Total-derived estimate needs the day basal (total - active), only known once every record is read. The existing HR and distance passes over `workouts` already work this way. Parity: Health Connect ingestion is Android-only. Checked for a Swift twin rather than assuming — the only hit is a comment reference in SleepMerge.swift. No obligation here. Tests: the six existing assertions are unchanged and still pass — proration and partial overlap are preserved deliberately, they are right for the uniform case. Eight added covering both bugs, both directions and the null paths. All ten verified against an independent implementation of the algorithm before commit. Not compiled or run by CI: android.yml is disabled. The helpers are pure and covered. Known limit: if a source writes only coarse daily records for BOTH streams the estimate is still a prorated slice. There is no data to do better with, and inventing a distribution would be a guess. Separately, if a comparison number is roughly one basal-hour higher, that is the active-vs-total distinction rather than an error. Effort-instead-of-calories, also requested in ryanbr#835, is split to ryanbr#843: it needs per-sample HR import, since the importer collapses HeartRateRecord to a daily average and writes no HR rows for StrainScorer to use.

ryanbr#846) Kotlin takes only the LAST /** */ before a declaration as its KDoc. Where two are stacked, the first silently becomes a dangling comment — invisible to Dokka and IDE hover, while still reading fine to a human scrolling past. That is why these survive review. HrvAnalyzer.classifyCoverage: the detached block is "Byte-parity twin of Swift classifyCoverage" — a parity-contract statement, and the parity rule is the first thing in CLAUDE.md. Merged into the KDoc alongside the NaN-comparison reasoning. PuffinExperiment.PREFS: two near-identical blocks, the first a strict subset of the second. Dropped. HealthConnectImporter: introduced in ryanbr#842 earlier today by inserting KcalRecord between maxSourceDouble KDoc and maxSourceDouble itself, so the comment documented the record and the function lost its docs. KcalRecord lifted above the KDoc. All three came from editing by insertion rather than in place. Provably comment-only: stripping comments and string literals leaves a byte-identical ORDERED token stream against main in all three files, so the compiler sees the same program. That matters here because android.yml is disabled and nothing compiles this. A repo-wide sweep reports zero remaining stacked sites, and zero of the sibling "doc block, blank line, declaration" pattern. Two historical copies survive in docs/superpowers/plans/ and are left alone — a dated plan records what was decided then, not a spec to keep current.

Kotlin and Swift attach a doc block to the declaration that FOLLOWS it, and keep only the LAST of several stacked ones. Insert a block above an existing one, or a declaration into the gap between a comment and the thing it documents, and the text is silently orphaned — still there, still above the right function, still correct prose, but no longer reaching Dokka or IDE hover. That is why review does not catch it. ryanbr#846 fixed three sites in one day, one of which ryanbr#842 had introduced that same morning, and the block that came adrift was "Byte-parity twin of Swift classifyCoverage" — a parity-contract statement, and parity is the first rule in CLAUDE.md. That is the case for gating rather than only fixing. Tools/doc_comment_lint.py catches both detaching patterns across Kotlin and Swift: stacked blocks, and a blank line between a doc block and its declaration. File headers (a blank line before package/import/@file:) are exempt, since that spacing is conventional and correct. Corrects ryanbr#846, which claimed the tree was clean afterwards. It was not: that sweep only matched SINGLE-LINE blocks. Handling multi-line blocks finds 25 more pre-existing sites across 13 files — 8 in WhoopBleClient.kt alone, and AppViewModel.kt:68 is a class doc detached by an inserted data class, the same shape as the ryanbr#842 slip. Those 25 are grandfathered per-file so the gate starts green and blocks only NEW ones. No existing code can red-check a contributor on a line they did not touch — the ryanbr#514 failure mode. Baselined by COUNT, not line number: line numbers churn on every edit above them, which keeps the file stale and trains people to regenerate it without reading it. A count moves only when someone adds or removes a site. It ratchets DOWN — fixing one prints an IMPROVED line asking for the allowance to be lowered. Verified five ways: passes on clean main; catches all three pre-ryanbr#846 sites when reconstructed; fails on a new site in an unbaselined file; fails on an EXTRA site in an already-baselined file (so the count is enforced, not mere presence); and clears nine false-positive probes including file headers, a doc above an annotation, and a `*/` appearing inside prose. Pure Python on ubuntu, six seconds. No Xcode or Gradle, so unlike the app-target and Android checks it actually runs while app-build.yml and android.yml are disabled. Its own job gates itself. New workflow rather than a step in i18n-coverage.yml: different concern, and that job is currently the only thing gating PRs — a docs regression should not report itself as an i18n failure. Not included: fixing the 25. Each needs a judgement call about how to merge two blocks, and doing 25 of those at volume is how the mistake gets made. The baseline makes them visible and burnable-down without blocking anyone. Note the 25 are machine-detected; three were spot-checked as genuine, the rest are grandfathered either way.

This was referenced Jul 26, 2026

[health-connect] Import per-sample HR, not just a daily average — unblocks Effort/zones/HR chart for strapless users #843

Open

Calories wrong for HealthConnect imported workouts vs effort score #835

Open

ryanbr merged commit 3aeba77 into main Jul 26, 2026
1 check passed

ryanbr deleted the fix/835-hc-workout-calories branch July 26, 2026 22:09

ryanbr mentioned this pull request Jul 26, 2026

docs: reattach three KDoc blocks that were documenting the wrong thing #846

Merged

ryanbr mentioned this pull request Jul 27, 2026

ci: gate on doc comments that bind to nothing #849

Merged

kvnloo mentioned this pull request Nov 4, 2026

test(android): pin Health Connect writer identity and interval coverage fixtures kvnloo/noop#127

Draft

Navigation Menu

Repository navigation

fix(health-connect): workout calories under-report, and double-count across sources - #842

fix(health-connect): workout calories under-report, and double-count across sources#842
ryanbr merged 3 commits into
mainfrom
fix/835-hc-workout-calories

ryanbr commented Jul 26, 2026

Uh oh!

ryanbr commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ryanbr commented Jul 26, 2026

1. Under-report

2. Double-count

Notes

What this does not fix

Uh oh!

ryanbr commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant