Skip to content

[health][parity] Body Clock ships on iOS/macOS but is inert on Android — the Kotlin twin is never fed #852

Closed

Description

@ryanbr

Found reviewing tanarchytan's fork, where they hit the same thing and wired it up. The finding transfers even though their implementation does not (their fork moved analytics onto a Rust FFI backend, which is incompatible with our parity contract — see below).

The card is dead

V5HealthSignals.kt:129-132:

// ── Body clock: needs per-hour rest-activity bins we don't bank here; the planner is on-demand.
//    Leave null so the BodyClockCard reads its honest "Calibrating" empty state until a future
//    activity-bin source lands (the engine is wired + ready, the input pipe is the gap). ──
val bodyClock: CircadianEngine.PhaseEstimate? = null

So BodyClockCard ships, is laid out, has its own offset/confidence copy and a jet-lag planner path — and shows "Calibrating" forever, on every device, because the value is a literal null.

CircadianEngine.estimatePhase is called by nothing but its own unit tests. The cosinor fit, the confidence bands and JetLagPlan are all implemented and covered.

The stated blocker looks stale

The comment says we do not bank per-hour rest-activity bins. ActivityBin is just (hour: Double, activity: Double) — an hourly activity magnitude. Upstream already banks per-second motion the whole day:

Hourly bins from step deltas or gravity deltas is a small aggregation, not a missing pipe. Whether the resulting phase estimate is good is a separate question the engine's own confidence bands already answer (UNREADABLE exists).

What I am not proposing

Not porting their code. Their fork replaced the Kotlin analytics with calls into a prebuilt libwhoop_ffi.so committed to the repo — that breaks the parity contract (Android is meant to be an independent reimplementation, not a shared binary), commits build artifacts, and makes the algorithms unauditable in-tree. None of that should come here.

Suggested shape

  1. Derive List<ActivityBin> from the motion already banked, over the trailing week.
  2. Pass it to estimatePhase in V5HealthSignals, and delete the stale comment.
  3. Let the existing confidence bands do the honest-data work — a thin week reads UNREADABLE, not a fabricated phase.
  4. Swift has no bodyClock at all, so parity means either building the twin or stating plainly that this is Android-only for now.

Either wire it or remove the card. Shipping a permanently-"Calibrating" card is the worst of the three, and it has apparently been that way long enough for the blocker to expire unnoticed.

Activity

  1. ryanbr commented on Jul 27, 2026

    @ryanbr
    OwnerAuthor

    Correcting this issue — I got the central claim wrong, and the correction shrinks it a lot.

    The card does not show "Calibrating". It does not render at all. HealthScreen.kt:458:

    // Body clock: only when the engine produced an estimate (no faked card while the input pipe is empty).
    signals?.bodyClock?.let { BodyClockCard(estimate = it) }

    bodyClock is always null, so the let never fires. There is no empty state for it — the only Calibrating strings in the Android tree belong to the Charge score. I took the wording of the V5HealthSignals comment ("reads its honest Calibrating empty state") at face value instead of checking the render site.

    So there is no user-visible defect, and the render site is deliberately correct: "no faked card while the input pipe is empty."

    And wiring it up — which I recommended — would be a mistake. Without an observedTempMinHour, estimatePhase derives the core-body-temperature minimum as activityAcrophase − 12.0h, a population constant, then reports "a night-owl lean" from it. The confidence bands do not cover that gap: SOLID/WIDE/UNREADABLE key on days observed and cosinor amplitude — data sufficiency, not whether the 12 h offset holds for this person. Fourteen clean days of wrist motion earns SOLID on a number resting on an assumption. That is the #194 pattern, and CLAUDE.md's rule about deriving physiological signals says not to ship it.

    What is actually left, in descending value:

    1. The comment is wrong and describes behaviour that does not happen. One-line fix, and the only thing here that is unambiguously worth doing.
    2. Dead code — ~200 lines of unreachable card UI plus a tested-but-uncalled engine. Deleting it is defensible; so is leaving it, since it is genuinely finished work someone may revive.
    3. If it is ever revived, feed observedTempMinHour from the skinTempSample we already bank per second and render only when that exists. That removes the population constant entirely and makes the estimate rest on something measured. That is a real feature, not a wiring job.

    Retitling to match. Apologies for the noise — the finding that the code is dead stands, the framing that it was user-visible did not.

  2. changed the title [-][health] The Body Clock card can never render — bodyClock is hardcoded null behind a blocker that no longer holds[/-] [+][health] Body Clock is dead code, and the comment describing it is wrong[/+] on Jul 27, 2026
  3. changed the title [-][health] Body Clock is dead code, and the comment describing it is wrong[/-] [+][health][parity] Body Clock ships on iOS/macOS but is inert on Android — the Kotlin twin is never fed[/+] on Jul 27, 2026
  4. ryanbr commented on Jul 27, 2026

    @ryanbr
    OwnerAuthor

    Third and final correction — the previous two were both wrong in the same direction, and the truth is the opposite of where I started.

    Body Clock is a working, shipped feature on iOS and macOS. AppModel.swift:1545 pools HR by local hour-of-day as the activity proxy, builds the bins, and calls the engine:

    let bins: [CircadianEngine.ActivityBin] = (0..<24).compactMap { … }
    guard bins.count >= 6 else { circadianPhase = nil; return }
    circadianPhase = CircadianEngine.estimatePhase(
        bins: bins, daysObserved: daySet.count, habitualWakeHour: wakeHour)

    AppModel publishes it, SkinTempCardsView.BodyClockCard renders it, and CircadianEngineTests covers it.

    So this is a parity gap, not dead code. Android has a byte-for-byte twin of the engine — the Kotlin file says exactly that in its header — full unit tests, and a finished card. The one missing piece is the twenty lines that build the bins. V5HealthSignals hardcodes bodyClock = null behind a comment claiming the input does not exist, and HealthScreen gates the card on a ?.let that therefore never fires.

    The comment is what misled me, and it is wrong. It says we need "per-hour rest-activity bins we don't bank here". Swift does not bank them either — it derives them from HR buckets Android also has. The pipe was never built on Android, not missing.

    What I got wrong, for the record: I first reported this as a user-visible "Calibrating" card (it renders nothing at all); then as dead code that should be deleted (#853, now closed); and I argued against reviving it on the grounds that the 12 h acrophase → CBTmin constant is unvalidated. That last point still stands but it applies to Apple too — so the answer is not to delete Android, it is to decide once, for both platforms.

    Two coherent options, and only these:

    1. Fix the parity gap — port the Swift HR-by-hour binning to Kotlin. Roughly twenty lines against an engine and a card that already exist and are already tested.
    2. Retire it on both platforms — if the population-constant derivation is not something we want to ship as a number, that judgement applies to the shipped Apple card first, not to the inert Android one.

    What is not defensible is the current state: a feature live on one platform, silently inert on the other, with a comment on the inert side asserting a blocker that the live side disproves.

  5. ryanbr commented on Jul 27, 2026

    @ryanbr
    OwnerAuthor

    Minimal fix for #852 — feed the Android Body Clock

    Two hunks. No new files, no engine change (it stays a byte-for-byte mirror), no schema,
    no migration, no new strings — the card and its copy already exist.

    1. V5HealthSignals.kt — accept the bins, drop the false comment

         fun evaluate(
             days: List<DailyMetric>,
             cycleOptedIn: Boolean,
             loggedPeriodStarts: List<String> = emptyList(),
             journalContext: IllnessSignalEngine.Context = IllnessSignalEngine.Context(),
             habitualWakeHour: Double = 7.0,
    +        activityBins: List<CircadianEngine.ActivityBin> = emptyList(),
    +        daysObserved: Int = 0,
         ): Snapshot {
    -        // ── Body clock: needs per-hour rest-activity bins we don't bank here; the planner is on-demand.
    -        //    Leave null so the BodyClockCard reads its honest "Calibrating" empty state until a future
    -        //    activity-bin source lands (the engine is wired + ready, the input pipe is the gap). ──
    -        val bodyClock: CircadianEngine.PhaseEstimate? = null
    +        // ── Body clock: the caller pools HR by local hour into `activityBins` (see AppViewModel), the
    +        //    same activity proxy the Swift twin uses in AppModel.computeCircadianPhase. Fewer than six
    +        //    populated hours is too thin to fit, so it stays null and HealthScreen's `?.let` skips the
    +        //    card — no faked estimate. Defaults keep every existing caller byte-identical. ──
    +        val bodyClock: CircadianEngine.PhaseEstimate? =
    +            if (activityBins.size >= 6) {
    +                CircadianEngine.estimatePhase(activityBins, daysObserved, habitualWakeHour)
    +            } else null

    2. AppViewModel.kt — build the bins, mirroring Swift exactly

    Placed here, not in CircadianEngine, because Swift builds them inline in AppModel —
    keeping the engine identical on both platforms is the point.

    /**
     * Per-hour activity profile for the body clock, from the last ~14 days of hourly HR buckets.
     * HR amplitude is a usable rest/activity rhythm proxy when raw motion isn't to hand.
     * Byte-for-byte mirror of Swift `AppModel.computeCircadianPhase` (bucketSeconds 3600, the
     * >= 24-bucket floor, local-hour pooling, and daysObserved as the count of distinct local days).
     * Returns empty when there is too little to read; the caller treats that as "no estimate".
     */
    private suspend fun circadianActivityBins(
        deviceId: String,
    ): Pair<List<CircadianEngine.ActivityBin>, Int> {
        val now = System.currentTimeMillis() / 1000L
        val from = now - 14L * 86_400L
        val buckets = runCatching { repository.hrBuckets(deviceId, from, now, 3_600L) }
            .getOrDefault(emptyList())
        if (buckets.size < 24) return emptyList<CircadianEngine.ActivityBin>() to 0
        val tz = TimeZone.getDefault().getOffset(now * 1000L) / 1000L
        val sums = DoubleArray(24)
        val counts = IntArray(24)
        val days = HashSet<Long>()
        for (b in buckets) {
            val local = b.bucket + tz
            val hour = (((local % 86_400L) + 86_400L) % 86_400L / 3_600L).toInt()
            sums[hour] += b.avgBpm
            counts[hour] += 1
            days.add(local / 86_400L)
        }
        val bins = (0 until 24).mapNotNull { h ->
            if (counts[h] > 0) CircadianEngine.ActivityBin(h.toDouble(), sums[h] / counts[h]) else null
        }
        return bins to days.size
    }

    and at the two V5HealthSignals.evaluate call sites (~:736 and ~:2147):

    +                    val (bins, observedDays) = circadianActivityBins(deviceId)
                         _v5Signals.value = V5HealthSignals.evaluate(
                             days = days,
                             cycleOptedIn = _cycleTrackingEnabled.value,
                             journalContext = illnessJournalContext(days),
    +                        activityBins = bins,
    +                        daysObserved = observedDays,
                         )

    What this deliberately does not do

    • No habitualWakeHour. Swift derives it from the most recent banked wake and falls back
      to 07:00. Kotlin has no equivalent helper, so this passes the existing 7.0 default. That is
      a real, bounded divergence: it shifts offsetVsScheduleMinutes for anyone whose habitual
      wake isn't 07:00. Worth a follow-up, but adding a wake-time derivation is a second change
      and would make this hunk twice the size for a value the engine already defaults.
    • No new tests for the engine — CircadianEngineTest already covers estimatePhase,
      cosinor and planShift. What is new is the binning, which is worth one unit test pinning
      the local-hour pooling and the >= 24 / >= 6 floors against a synthetic bucket series.

    The open question this does not settle

    Wiring Android makes both platforms consistent. It does not address whether the estimate
    should ship at all: without an observedTempMinHour, estimatePhase derives the CBT minimum
    as activityAcrophase − 12.0h, a population constant, and the confidence bands key on data
    sufficiency rather than on whether that constant holds. That question is about the already
    shipped Apple card
    , and it should be answered before or alongside this — not by leaving
    Android silently inert, which is what the current state does.

  6. ryanbr commented on Jul 27, 2026

    @ryanbr
    OwnerAuthor

    Merged as ce94e69b (#855). Android now computes the estimate from the same HR-by-local-hour profile the Swift twin has used all along, via the #814 union read.

    Two things this deliberately did not settle, both recorded on the PR:

    1. habitualWakeHour — Swift derives it from the most recent banked wake, Kotlin uses the 7.0 default. That shifts offsetVsScheduleMinutes for anyone who does not wake at 07:00. Small, bounded, worth a follow-up.
    2. Whether the estimate should ship at all. Without an observedTempMinHour it derives the CBT minimum as activityAcrophase − 12.0h, a population constant, and the confidence bands key on data sufficiency rather than on whether that constant holds for this person. That is a question about the already-shipped Apple card, and it is now a single decision affecting both platforms rather than one shipped and one inert.

    Closing this, since the parity gap it describes is fixed. Point 2 deserves its own issue when you want to take it up — the honest route there is feeding observedTempMinHour from the skinTempSample already banked per second, so the phase rests on a measurement rather than a constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions