Repository navigation
coach: send the sleep stages it already has - #1817
Merged
Merged
Conversation
A user asked why the coach said it could not access their sleep stage data. It was telling the truth: the context carried "rest 7.5h" and nothing else about a night. No deep, no REM, no light, no efficiency - the model was not refusing or inventing a limit, it genuinely had none of it. The data was never missing. deepMin, remMin, lightMin and efficiency sit on the SAME DailyMetric row the day line already reads for totalSleepMin, avgHrv and restingHr. They were simply never included, which looks like an oversight rather than a decision - #124 widened this same context once before for the same reason ("the coach used to see only recovery/strain/sleep/HRV/RHR"). Both platforms, since AiCoach.kt is a twin of AICoach.swift and had the identical gap. Always emitted, dash when absent, exactly like every other field on the line. The alternative - appending stages only when present - gives the model a schema that changes shape between days and invites it to read a missing field as a zero, which is the one misreading worth designing against for a user whose strap banks no staging at all. Both system prompts updated too. Each ENUMERATES what the coach may be given, so adding the data without the prompt would have left the model told it does not have what it just received. The Swift prompt also now says a dash means NOT MEASURED rather than zero; the Kotlin context header already said so. One pre-existing test repinned rather than weakened: it asserts one contiguous daily line where every unrecorded value is a dash, and that string now carries the new fields. Contiguous is the point - it is what catches a field silently dropping out. 14 AiCoach tests, 0 failures; compileFullDebugKotlin, swiftc -parse, doc_comment_lint and i18n_audit all clean. The Swift half is app-target with no local build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RupifTqaTTytpPjfpMvJuj
…eads 9400%
Re-review, and this one would have shipped. DailyMetric.efficiency is NOT reliably a 0-1 fraction: it
"arrives as % on some import paths", which SleepView and StagesCard each guard against inline with
the same `if e > 1.5 { e /= 100 }` test, and which SleepMetricDetailLogic and SleepModelLogic guard
against on Android. My bare `* 100` would have made the coach's line read "eff 9400%" for any
imported night - and a model handed a nonsense number reasons about it confidently rather than
ignoring it, which is worse than the missing field this PR set out to fix.
Both platforms normalise first now. I would have been the fifth site needing this defence and the
only one without it; a shared helper is clearly warranted, but extracting one touches four unrelated
files and does not belong in a coach change.
Threshold picked deliberately rather than copied: 1.5, matching Swift's two copies. A genuine
fraction can exceed 1.0 only by floating-point noise while a genuine percentage is 30-100, so 1.5 is
the safer split. Android's existing copies use 1.0 instead - a pre-existing cross-platform divergence
in the same guard, left alone here beyond keeping the COACH line consistent across both.
Pinned by a test that feeds the same night as 0.94 and as 94.0 and asserts both render "eff 94%", and
that "9400%" never appears.
15 AiCoach tests, 0 failures; kotlin compiles, swiftc -parse clean, both lints clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RupifTqaTTytpPjfpMvJuj
Re-review pass two. Fifteen Kotlin tests covered this change and zero Swift ones, for a change that is deliberately a twin - including the efficiency normalisation that pass one caught, which is exactly the logic worth pinning on both sides rather than one. The reason for the gap is structural, not laziness: Swift's buildContext() takes no arguments and reads the repo, while the Kotlin twin is handed the day list, so the Swift formatter had no seam a test could reach and dayLine was private. Widened dayLine and efficiencyPercentOrDash to internal - both are pure - and added AICoachSleepContextTests mirroring the Kotlin cases: stages reach the line, an unstaged night reports dashes, and the same night as 0.94 and as 94.0 both render "eff 94%". Also pinned 1.2 rendering as a dash. It sits above a fraction and below the percentage split, so it is not a value this can honestly render, and asserting that stops a future widening of the threshold passing silently. The DailyMetric initialiser needed `disturbances` and `exerciseCount` too - caught by reading the signature rather than by a compiler, since StrandTests only build under app-build. That is also the honest caveat on these tests: they are written but unrun, and app-build is what executes them. Kotlin 15 tests, 0 failures; swiftc -parse clean on both files; both lints clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RupifTqaTTytpPjfpMvJuj
ksricharank
pushed a commit
to ksricharank/noop
that referenced
this pull request
Sep 8, 2026
The v11.1.0 uplift found upstream had independently built the same feature the fork shipped as ryanbr#1817 — richer sleep detail in the LLM context — with the opposite rule. The fork emits stages ALWAYS, "—" when the night did not record them; upstream put them behind the `includeDerivedTrends` toggle and omitted them when nil. Both had tests; both could not hold. Kept the fork's rule, per maintainer decision at the uplift: a fixed schema is what stops the model inferring. A line whose columns appear and vanish between days invites "no deep field" to be read as zero — the same misreading upstream's omit rule guards against, arriving by the other door. "deep —" states the absence out loud. So the narrow line keeps deep/REM/light/eff in hours, and the genuinely wide-only columns (wakes, SDNN, skin) keep upstream's omit-when-nil rule: they are extra detail rather than part of a night's basic shape, so their absence carries no meaning. Kept from upstream's side: the static, engine-free `dayLine(_:wide:)` seam (its helpers became static with it), the bimodal skin-temperature labelling, and the `derivedTrendsBlock` tests. Upstream's narrow-line assertion is INVERTED rather than deleted — the property it guards, no wide-only leakage into the default context, still matters and is still pinned. Its wide-line test now also asserts stages are not re-emitted in minutes beside the hours, which the naive merge did. Android parity: `AiCoach.kt` already emits stages unconditionally in hours with a "-" fallback, so the Kotlin twin matches the kept rule with no change. Also fixes a pre-existing wrong expectation in DailyTargetsTests: the sleep-need case omitted the debt term, asserting 515 against a correct 520. The code was right; the arithmetic in the test was short by one term. It predates this uplift. Verified: Strand (macOS) and NOOPiOS both BUILD SUCCEEDED; StrandTests 1666 with only the known timezone-sensitive SkinTempAbsoluteDisplayTests flake; all four Swift packages green (WhoopProtocol 654, WhoopStore 476, StrandAnalytics 1763, StrandImport 249). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ksricharank
pushed a commit
to ksricharank/noop
that referenced
this pull request
Sep 9, 2026
The v11.1.0 uplift found upstream had independently built the same feature the fork shipped as ryanbr#1817 — richer sleep detail in the LLM context — with the opposite rule. The fork emits stages ALWAYS, "—" when the night did not record them; upstream put them behind the `includeDerivedTrends` toggle and omitted them when nil. Both had tests; both could not hold. Kept the fork's rule, per maintainer decision at the uplift: a fixed schema is what stops the model inferring. A line whose columns appear and vanish between days invites "no deep field" to be read as zero — the same misreading upstream's omit rule guards against, arriving by the other door. "deep —" states the absence out loud. So the narrow line keeps deep/REM/light/eff in hours, and the genuinely wide-only columns (wakes, SDNN, skin) keep upstream's omit-when-nil rule: they are extra detail rather than part of a night's basic shape, so their absence carries no meaning. Kept from upstream's side: the static, engine-free `dayLine(_:wide:)` seam (its helpers became static with it), the bimodal skin-temperature labelling, and the `derivedTrendsBlock` tests. Upstream's narrow-line assertion is INVERTED rather than deleted — the property it guards, no wide-only leakage into the default context, still matters and is still pinned. Its wide-line test now also asserts stages are not re-emitted in minutes beside the hours, which the naive merge did. Android parity: `AiCoach.kt` already emits stages unconditionally in hours with a "-" fallback, so the Kotlin twin matches the kept rule with no change. Also fixes a pre-existing wrong expectation in DailyTargetsTests: the sleep-need case omitted the debt term, asserting 515 against a correct 520. The code was right; the arithmetic in the test was short by one term. It predates this uplift. Verified: Strand (macOS) and NOOPiOS both BUILD SUCCEEDED; StrandTests 1666 with only the known timezone-sensitive SkinTempAbsoluteDisplayTests flake; all four Swift packages green (WhoopProtocol 654, WhoopStore 476, StrandAnalytics 1763, StrandImport 249). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ksricharank
pushed a commit
to ksricharank/noop
that referenced
this pull request
Sep 12, 2026
The v11.1.0 uplift found upstream had independently built the same feature the fork shipped as ryanbr#1817 — richer sleep detail in the LLM context — with the opposite rule. The fork emits stages ALWAYS, "—" when the night did not record them; upstream put them behind the `includeDerivedTrends` toggle and omitted them when nil. Both had tests; both could not hold. Kept the fork's rule, per maintainer decision at the uplift: a fixed schema is what stops the model inferring. A line whose columns appear and vanish between days invites "no deep field" to be read as zero — the same misreading upstream's omit rule guards against, arriving by the other door. "deep —" states the absence out loud. So the narrow line keeps deep/REM/light/eff in hours, and the genuinely wide-only columns (wakes, SDNN, skin) keep upstream's omit-when-nil rule: they are extra detail rather than part of a night's basic shape, so their absence carries no meaning. Kept from upstream's side: the static, engine-free `dayLine(_:wide:)` seam (its helpers became static with it), the bimodal skin-temperature labelling, and the `derivedTrendsBlock` tests. Upstream's narrow-line assertion is INVERTED rather than deleted — the property it guards, no wide-only leakage into the default context, still matters and is still pinned. Its wide-line test now also asserts stages are not re-emitted in minutes beside the hours, which the naive merge did. Android parity: `AiCoach.kt` already emits stages unconditionally in hours with a "-" fallback, so the Kotlin twin matches the kept rule with no change. Also fixes a pre-existing wrong expectation in DailyTargetsTests: the sleep-need case omitted the debt term, asserting 515 against a correct 520. The code was right; the arithmetic in the test was short by one term. It predates this uplift. Verified: Strand (macOS) and NOOPiOS both BUILD SUCCEEDED; StrandTests 1666 with only the known timezone-sensitive SkinTempAbsoluteDisplayTests flake; all four Swift packages green (WhoopProtocol 654, WhoopStore 476, StrandAnalytics 1763, StrandImport 249). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ksricharank
pushed a commit
to ksricharank/noop
that referenced
this pull request
Sep 19, 2026
The v11.1.0 uplift found upstream had independently built the same feature the fork shipped as ryanbr#1817 — richer sleep detail in the LLM context — with the opposite rule. The fork emits stages ALWAYS, "—" when the night did not record them; upstream put them behind the `includeDerivedTrends` toggle and omitted them when nil. Both had tests; both could not hold. Kept the fork's rule, per maintainer decision at the uplift: a fixed schema is what stops the model inferring. A line whose columns appear and vanish between days invites "no deep field" to be read as zero — the same misreading upstream's omit rule guards against, arriving by the other door. "deep —" states the absence out loud. So the narrow line keeps deep/REM/light/eff in hours, and the genuinely wide-only columns (wakes, SDNN, skin) keep upstream's omit-when-nil rule: they are extra detail rather than part of a night's basic shape, so their absence carries no meaning. Kept from upstream's side: the static, engine-free `dayLine(_:wide:)` seam (its helpers became static with it), the bimodal skin-temperature labelling, and the `derivedTrendsBlock` tests. Upstream's narrow-line assertion is INVERTED rather than deleted — the property it guards, no wide-only leakage into the default context, still matters and is still pinned. Its wide-line test now also asserts stages are not re-emitted in minutes beside the hours, which the naive merge did. Android parity: `AiCoach.kt` already emits stages unconditionally in hours with a "-" fallback, so the Kotlin twin matches the kept rule with no change. Also fixes a pre-existing wrong expectation in DailyTargetsTests: the sleep-need case omitted the debt term, asserting 515 against a correct 520. The code was right; the arithmetic in the test was short by one term. It predates this uplift. Verified: Strand (macOS) and NOOPiOS both BUILD SUCCEEDED; StrandTests 1666 with only the known timezone-sensitive SkinTempAbsoluteDisplayTests flake; all four Swift packages green (WhoopProtocol 654, WhoopStore 476, StrandAnalytics 1763, StrandImport 249). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
A user asked why the coach said it couldn't access their sleep stage data. It was telling the
truth.
What the coach could see
rest 7.5hwas every word the context carried about a night. No deep, no REM, no light, noefficiency. The model wasn't refusing or inventing a limitation — it genuinely had none of it.
The data was never missing
deepMin,remMin,lightMinandefficiencysit on the sameDailyMetricrow the day linealready reads for
totalSleepMin,avgHrvandrestingHr. Nothing new is plumbed; they were simplynever included.
That looks like an oversight rather than a decision — #124 widened this same context once before, for
the same reason ("the coach used to see only recovery/strain/sleep/HRV/RHR").
Both platforms, since
AiCoach.ktis a twin ofAICoach.swiftand had the identical gap.Design notes
Always emitted, dash when absent, exactly like every other field on the line. The alternative —
appending stages only when present — gives the model a schema that changes shape between days and
invites it to read a missing field as a zero. That is the one misreading worth designing against,
because the users most likely to ask about stages are the ones whose strap banks none.
Both system prompts updated too. Each enumerates what the coach may be given, so adding the data
without the prompt would have left the model explicitly told it doesn't have what it just received.
The Swift prompt also now says a dash means NOT MEASURED rather than zero; the Kotlin context header
already said so.
One test repinned, not weakened
sparseRawOnlyDaysRenderDashesNotInventedNumbersasserts one contiguous daily line where everyunrecorded value is a dash. That string now carries the new fields. Contiguous is the point — it's
what catches a field silently dropping out of the line.
Verification
night reports dashes rather than going quiet.
compileFullDebugKotlin,swiftc -parse,doc_comment_lint,i18n_audit --ciall clean.🤖 Generated with Claude Code