From 7cdd79648b20933375f8f3ae582367a622781e41 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Sat, 29 Aug 2026 12:33:11 +1200 Subject: [PATCH] Make the two hypnogram-coverage readers agree, and state the gate's real scope Follow-up to #1717, from reviewing it. The coverage gate is pinned by an oracle across 18 payload shapes, and the two readers still disagreed on shapes the oracle did not enumerate. Swift decodes the stage array as a WHOLE (`as? [[String: Any]]`), so one non-object element makes the entire payload unmeasurable; Kotlin skipped per element and measured the remainder. Kotlin's `optDouble` also parsed a string bound that Swift's `as? NSNumber` rejects. Compiled both and ran them: [{seg},5] Swift null/not-holed Kotlin 0.1/HOLED [{seg},null] Swift null/not-holed Kotlin 0.1/HOLED [{"start":"0","end":"100"}] Swift null/not-holed Kotlin 0.1/HOLED Unreachable from any current producer - every writer emits homogeneous segments or a timestamp-free shape - so this is latent, not a live defect. It is still a divergence in the one module whose contract is byte-parity, and the oracle would never have caught it. Kotlin now bails on a non-object element and reads bounds the way NSNumber does. Both directions were checked against the real Swift, not against my reading of it: a bool bound CONVERTS on both sides, because JSONSerialization bridges `true` to NSNumber 1, and the tidy-looking alignment of rejecting non-numbers would have been wrong - under Foundation `{"start":0}` reports `is Bool == true`, so a bool exclusion would have discarded every timeline whose first segment starts at zero. Five rows added to the oracle. Two scope corrections, both documentation: - The module said the gate stays away from imports because their minute-dict shape has no timestamps. True for the WHOOP CSV, Apple and Health Connect, but not a blanket exemption: the Xiaomi Band importer emits real {start,end,stage} segments and takes its span from bedtime/wake_up_time fields independent of the items array it builds them from, so a Xiaomi night IS judged here. Arguably right - such a night genuinely is only partly described - but it is a real behaviour change for that importer, unvalidated against an export, and now pinned by test rather than left to be found. - At the ENGINE call site coverage is summed off DECODED segments, not off stagesJSON, so the payload-shape rule never runs there. What actually keeps timestamp-free imports safe is `coveredSeconds > 0` returning nil for a group with no timestamped stages at all. Same outcome, different mechanism, and it holds only while a group is single-sourced - which the day merge ensures. Re-review added two more rows, both shapes previously argued rather than run. Health Connect's REAL payload is an ARRAY of {stage,min} objects, not the throwing {light,deep,rem,awake} dict the oracle already had: it parses, reaches the loop, and exits with zero cover. That is the live producer shape this gate claims exemption from, so it is now pinned along its actual path rather than by assertion. A JSON null bound inside a well-formed object is pinned beside it. The inline comment naming only the throwing dict was reconciled with the doc. No behaviour change for any payload a producer can emit. Android 4752 pass. Swift package tests cannot build on this box (GRDB CSQLite), so the Swift half is syntax-checked locally and rests on CI; the new Swift expectations were generated by running the twin standalone rather than by hand. --- .../StrandAnalytics/AnalyticsEngine.swift | 8 ++ .../WhoopStore/HypnogramCoverage.swift | 18 ++++- .../HypnogramCoverageTests.swift | 78 +++++++++++++++++++ .../com/noop/analytics/AnalyticsEngine.kt | 8 ++ .../com/noop/analytics/HypnogramCoverage.kt | 55 ++++++++++--- .../noop/analytics/HypnogramCoverageTest.kt | 48 ++++++++++++ 6 files changed, 201 insertions(+), 14 deletions(-) diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift index 6a4be87dea..8ae469a42f 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift @@ -553,6 +553,14 @@ public enum AnalyticsEngine { // observed at all. Summed straight off `s.stages` (no JSON re-parse: the segments are already // decoded here), then handed to `HypnogramCoverage` so the ratio/clamp/nil rules have ONE // definition shared with the merge-side guard. + // NOTE on scope at THIS call site: coverage here is summed off the DECODED segments, not off + // `stagesJSON`, so the timestamp-free shapes are not screened out by the payload-shape rule the + // merge side uses. A minute-dict session decodes to zero segments and contributes span with no + // cover, and what keeps it from reading as a holed night is `fraction`'s `coveredSeconds > 0` + // returning nil for a group with no timestamped stages at all. Same outcome, different + // mechanism — and it holds only while a group is single-sourced, which the day merge ensures by + // choosing one side. A group mixing a timestamped fragment with a minute-dict one would read as + // holed; `HypnogramCoverageTests.testMixedSourceGroupReadsAsHoled` pins both halves. var coveredS = 0.0, spanS = 0.0 for s in mainGroup { let m = SleepStager.hypnogramMetrics(s) diff --git a/Packages/WhoopStore/Sources/WhoopStore/HypnogramCoverage.swift b/Packages/WhoopStore/Sources/WhoopStore/HypnogramCoverage.swift index ff48cda5c9..5b848ce22c 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/HypnogramCoverage.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/HypnogramCoverage.swift @@ -52,10 +52,20 @@ public enum HypnogramCoverage { /// is not a meaningful question for that payload. /// /// nil for: a nil/blank/`"[]"` payload (no stages at all — that is the richness question, not this - /// one) and for the IMPORTED minute-dict shape `{light,deep,rem,awake}`, which carries no - /// timestamps and therefore cannot be compared against a span. That second case is what keeps this - /// gate away from WHOOP/Apple/Health-Connect imports entirely: they are never judged incomplete - /// here, so no existing import behaviour changes. + /// one), for the IMPORTED minute-dict shape `{light,deep,rem,awake}` and for Health Connect's + /// `{stage,min}` array, neither of which carries timestamps to compare against a span, and for an + /// array holding any non-object element (see the Kotlin twin's loop for why that bails rather than + /// measuring the remainder). + /// + /// SCOPE, precisely. Timestamp-free shapes are what keep this gate off the WHOOP CSV, Apple and + /// Health Connect imports — they are never judged incomplete, so their behaviour is unchanged. That + /// is NOT a blanket exemption for imports, and it was originally written as one: the Xiaomi Band + /// importer emits real `{start,end,stage}` segments, and takes its span from `bedtime`/`wake_up_time` + /// fields that are independent of the `items` array it builds those segments from. A Xiaomi night + /// whose items do not reach its own bed/wake bounds IS judged holed here. That is arguably the right + /// answer — the night genuinely is only partly described — but it is a real behaviour change for + /// that importer, unvalidated against a Xiaomi export, and it is pinned by test rather than left to + /// be discovered. public static func fraction(stagesJSON: String?, spanSeconds: Double) -> Double? { guard let json = stagesJSON?.trimmingCharacters(in: .whitespacesAndNewlines), !json.isEmpty, json != "[]", diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/HypnogramCoverageTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/HypnogramCoverageTests.swift index 95978e175d..e00cfdfcce 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/HypnogramCoverageTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/HypnogramCoverageTests.swift @@ -98,4 +98,82 @@ final class HypnogramCoverageTests: XCTestCase { XCTAssertFalse(HypnogramCoverage.isHoled(whole)) XCTAssertFalse(HypnogramCoverage.isHoled(stageless)) } + + // MARK: - shapes the two readers must agree on + + /// The four payloads on which Swift and Kotlin originally DISAGREED, found by compiling both and + /// running them rather than by reading them side by side. Swift's whole-array cast makes one + /// non-object element poison the payload; the Kotlin twin used to skip that element and measure the + /// remainder, reading 0.1 and HOLED where this side read nil and not-holed. It now bails the same + /// way. Kept here so the agreement is asserted from BOTH sides, not only in the Kotlin oracle. + func testNonObjectElementMakesThePayloadUnmeasurable() { + for json in [#"[{"start":0,"end":100,"stage":"deep"},5]"#, + #"[{"start":0,"end":100,"stage":"deep"},null]"#, + #"[{"start":0,"end":100,"stage":"deep"},"x"]"#] { + XCTAssertNil(HypnogramCoverage.fraction(stagesJSON: json, spanSeconds: 1000), + "one non-object element must make the whole payload unmeasurable") + XCTAssertFalse(HypnogramCoverage.isHoled(stagesJSON: json, spanSeconds: 1000)) + } + } + + /// A string-valued bound is SKIPPED, not parsed: `NSString` is not an `NSNumber`. The Kotlin twin + /// reached the same answer only after dropping `optDouble`, which parses `"0"` happily. + func testStringBoundsAreNotCounted() { + let json = #"[{"start":"0","end":"100","stage":"deep"}]"# + XCTAssertNil(HypnogramCoverage.fraction(stagesJSON: json, spanSeconds: 1000)) + } + + /// A bool bound DOES convert, on both sides — `JSONSerialization` bridges `true` to `NSNumber` 1. + /// Absurd in a stage payload and unreachable from any producer, pinned because it is the one case + /// where the tidy-looking alignment (reject anything that is not a number) would have been wrong: + /// under Foundation `{"start":0}` reports `is Bool == true`, so a bool exclusion would have thrown + /// away every timeline whose first segment starts at zero. + func testBoolBoundConvertsOnBothSides() { + let json = #"[{"start":true,"end":100,"stage":"deep"}]"# + XCTAssertEqual(HypnogramCoverage.fraction(stagesJSON: json, spanSeconds: 1000)!, 0.099, accuracy: 1e-12) + } + + /// SCOPE: a timestamped import is judged like any other timeline. This is the Xiaomi Band shape — + /// real `{start,end,stage}` segments whose span comes from separate bed/wake fields — and it is the + /// one importer this gate reaches. Timestamp-free imports stay exempt, asserted above. + func testTimestampedImportIsInScope() { + let json = segs([(0, 3600, "light")]) + XCTAssertEqual(HypnogramCoverage.fraction(stagesJSON: json, spanSeconds: 28800)!, 0.125, accuracy: 1e-12) + XCTAssertTrue(HypnogramCoverage.isHoled(stagesJSON: json, spanSeconds: 28800)) + } + + /// The ENGINE call site sums coverage off decoded segments, not off `stagesJSON`, so the payload + /// shape rule that exempts timestamp-free imports at the merge does not run there. What actually + /// protects them is this: a group with no timestamped stages at all covers zero, and zero cover is + /// unmeasurable rather than bad. Holds only while a group is single-sourced — a group mixing a + /// staged fragment with a minute-dict one covers part of its total span and reads as holed. + func testGroupWithNoTimestampedStagesIsUnmeasurableNotHoled() { + XCTAssertNil(HypnogramCoverage.fraction(coveredSeconds: 0, spanSeconds: 8 * 3600)) + } + + func testMixedSourceGroupReadsAsHoled() { + // 8 h fully-staged fragment + a 2 h minute-dict fragment that decodes to no segments. + let covered = 8.0 * 3600, span = 10.0 * 3600 + XCTAssertEqual(HypnogramCoverage.fraction(coveredSeconds: covered, spanSeconds: span)!, 0.8, accuracy: 1e-12) + XCTAssertLessThan(0.8, HypnogramCoverage.minCoverage) + } + + /// Health Connect's REAL payload shape. The `minute-dict` case above is the throwing + /// `{light,deep,rem,awake}` object; HC emits an ARRAY of `{stage,min}` objects, which parses + /// cleanly and reaches the segment loop. It is the live producer shape this gate claims to be + /// exempt from, so it is pinned rather than argued: every element is an object (no whole-cast + /// failure), none carries bounds (every segment skipped), cover stays 0, and zero cover is + /// unmeasurable rather than bad. + func testHealthConnectStageMinArrayIsUnmeasurable() { + let json = #"[{"stage":"light","min":300},{"stage":"deep","min":100}]"# + XCTAssertNil(HypnogramCoverage.fraction(stagesJSON: json, spanSeconds: 3600)) + XCTAssertFalse(HypnogramCoverage.isHoled(stagesJSON: json, spanSeconds: 3600)) + } + + /// A JSON null BOUND inside an otherwise well-formed object: the object is fine, so neither side + /// bails; both fail to read the bound and skip the segment. + func testNullBoundSkipsTheSegment() { + XCTAssertNil(HypnogramCoverage.fraction(stagesJSON: #"[{"start":null,"end":100,"stage":"deep"}]"#, + spanSeconds: 1000)) + } } diff --git a/android/app/src/main/java/com/noop/analytics/AnalyticsEngine.kt b/android/app/src/main/java/com/noop/analytics/AnalyticsEngine.kt index dfecb7be4d..f9b9d97935 100644 --- a/android/app/src/main/java/com/noop/analytics/AnalyticsEngine.kt +++ b/android/app/src/main/java/com/noop/analytics/AnalyticsEngine.kt @@ -502,6 +502,14 @@ object AnalyticsEngine { // observed at all. Summed straight off `s.stages` (no JSON re-parse: the segments are already // decoded here), then handed to HypnogramCoverage so the ratio/clamp/null rules have ONE // definition shared with the merge-side guard. Mirrors Swift. + // NOTE on scope at THIS call site: coverage here is summed off the DECODED segments, not off + // `stagesJSON`, so the timestamp-free shapes are not screened out by the payload-shape rule the + // merge side uses. A minute-dict session decodes to zero segments and contributes span with no + // cover, and what keeps it from reading as a holed night is `fraction`'s `coveredSeconds > 0` + // returning nil for a group with no timestamped stages at all. Same outcome, different + // mechanism — and it holds only while a group is single-sourced, which the day merge ensures by + // choosing one side. A group mixing a timestamped fragment with a minute-dict one would read as + // holed; `HypnogramCoverageTest.mixedSourceGroupReadsAsHoled` pins both halves. var coveredS = 0.0 var spanS = 0.0 for (s in mainGroup) { diff --git a/android/app/src/main/java/com/noop/analytics/HypnogramCoverage.kt b/android/app/src/main/java/com/noop/analytics/HypnogramCoverage.kt index c5f8892b45..9c9f072022 100644 --- a/android/app/src/main/java/com/noop/analytics/HypnogramCoverage.kt +++ b/android/app/src/main/java/com/noop/analytics/HypnogramCoverage.kt @@ -62,29 +62,64 @@ object HypnogramCoverage { * not a meaningful question for that payload. * * null for: a null/blank/`"[]"` payload (no stages at all — that is the richness question, not this - * one) and for the IMPORTED minute-dict shape `{light,deep,rem,awake}`, which carries no timestamps - * and therefore cannot be compared against a span. That second case is what keeps this gate away from - * WHOOP/Apple/Health-Connect imports entirely: they are never judged incomplete here, so no existing - * import behaviour changes. + * one), for the IMPORTED minute-dict shape `{light,deep,rem,awake}` and for Health Connect's + * `{stage,min}` array, neither of which carries timestamps to compare against a span, and for an + * array holding any non-object element (see the loop below for why that bails rather than measuring + * the remainder). + * + * SCOPE, precisely. Timestamp-free shapes are what keep this gate off the WHOOP CSV, Apple and + * Health Connect imports — they are never judged incomplete, so their behaviour is unchanged. That + * is NOT a blanket exemption for imports, and it was originally written as one: the Xiaomi Band + * importer emits real `{start,end,stage}` segments, and takes its span from `bedtime`/`wake_up_time` + * fields that are independent of the `items` array it builds those segments from. A Xiaomi night + * whose items do not reach its own bed/wake bounds IS judged holed here. That is arguably the right + * answer — the night genuinely is only partly described — but it is a real behaviour change for + * that importer, unvalidated against a Xiaomi export, and it is pinned by test rather than left to + * be discovered. */ fun fraction(stagesJson: String?, spanSeconds: Double): Double? { val json = stagesJson?.trim() ?: return null if (json.isEmpty() || json == "[]") return null - // The dict shape (imported minute totals) throws here and falls through to null — deliberately, + // The minute-dict shape (imported totals) throws here and falls through to null — deliberately, // since it carries no timestamps to measure. Same try/catch idiom as SleepStageTotals.minutes. + // Health Connect's `{stage,min}` array does NOT throw: it parses, reaches the loop, and every + // segment is skipped for want of bounds, so it exits with zero cover — unmeasurable by the same + // rule, along a different path. Both are pinned in the oracle. val arr = try { JSONArray(json) } catch (_: Throwable) { return null } var covered = 0.0 for (i in 0 until arr.length()) { - val seg = arr.optJSONObject(i) ?: continue - if (!seg.has("start") || !seg.has("end")) continue - val s = seg.optDouble("start", Double.NaN) - val e = seg.optDouble("end", Double.NaN) - if (s.isNaN() || e.isNaN() || e <= s) continue + // The Swift twin decodes the array as a WHOLE (`as? [[String: Any]]`), so ONE non-object + // element makes the entire payload unmeasurable there. Bail identically rather than + // measuring the remainder. Measured on the shipped pair, `[{seg},5]` and `[{seg},null]` + // read nil/not-holed on Swift and 0.1/HOLED here — and since this gate can only downgrade + // a night, refusing to judge a payload we do not fully understand is the safe half of that + // disagreement, and the one the module's fail-OPEN rule already commits to. + val seg = arr.optJSONObject(i) ?: return null + val s = num(seg.opt("start")) ?: continue + val e = num(seg.opt("end")) ?: continue + if (e <= s) continue covered += e - s } return fraction(covered, spanSeconds) } + /** + * A JSON scalar read the way Swift's `(seg["start"] as? NSNumber)?.doubleValue` reads it. + * + * A number converts. A STRING does not — `NSString` is not an `NSNumber`, so the Swift side skips + * that segment, whereas `optDouble` would have parsed it and counted the segment here. A BOOL does + * convert, because `JSONSerialization` bridges `true`/`false` to `NSNumber` and yields 1/0; that is + * absurd in a stage payload and no producer emits it, but the two readers must not disagree + * anywhere either could be asked. Verified against the Swift twin compiled standalone rather than + * read off the page: `{"start":0}` reports `is Bool == true` under Foundation, so a bool EXCLUSION + * would have rejected every timeline whose first segment starts at zero. + */ + private fun num(v: Any?): Double? = when (v) { + is Number -> v.toDouble() + is Boolean -> if (v) 1.0 else 0.0 + else -> null + } + /** * True when the timeline is known to cover less than [minCoverage] of its span — i.e. the session * demonstrably describes only part of the night it claims. Unknown coverage (null) is NOT holed: diff --git a/android/app/src/test/java/com/noop/analytics/HypnogramCoverageTest.kt b/android/app/src/test/java/com/noop/analytics/HypnogramCoverageTest.kt index b91d1385f3..436ede1baf 100644 --- a/android/app/src/test/java/com/noop/analytics/HypnogramCoverageTest.kt +++ b/android/app/src/test/java/com/noop/analytics/HypnogramCoverageTest.kt @@ -43,6 +43,13 @@ class HypnogramCoverageTest { minute-dict|null|false zero-span|null|false neg-span|null|false + mixed-nonobject|null|false + array-with-null|null|false + string-numbers|null|false + bool-start|0.099|true + timestamped-import|0.125|true + hc-stage-min|null|false + null-bound|null|false """.trimIndent() private fun seg(vararg r: Pair): String = @@ -68,6 +75,30 @@ class HypnogramCoverageTest { Triple("minute-dict", """{"light":300,"deep":100,"rem":80,"awake":40}""", 3600.0), Triple("zero-span", seg(0 to 300), 0.0), Triple("neg-span", seg(0 to 300), -1.0), + // The four shapes on which the two readers originally DISAGREED, measured rather than read: + // Swift's whole-array cast returns nil for a non-object element, where this side used to skip + // it and measure the remainder (0.1, HOLED). A string start used to parse via optDouble and + // count; Swift's `as? NSNumber` rejects it. A bool start converts on BOTH sides, because + // JSONSerialization bridges it to NSNumber — the one case where "reject anything odd" would + // have been the wrong alignment, and `{"start":0}` reporting `is Bool == true` under Foundation + // is why a bool exclusion could not be used to express it. + Triple("mixed-nonobject", """[{"start":0,"end":100,"stage":"deep"},5]""", 1000.0), + Triple("array-with-null", """[{"start":0,"end":100,"stage":"deep"},null]""", 1000.0), + Triple("string-numbers", """[{"start":"0","end":"100","stage":"deep"}]""", 1000.0), + Triple("bool-start", """[{"start":true,"end":100,"stage":"deep"}]""", 1000.0), + // A Xiaomi-shaped import: real timestamped segments that do not reach the session's own + // bed/wake span. Unlike the timestamp-free imports this IS judged, which is the scope the + // module doc now states outright. + Triple("timestamped-import", seg(0 to 3600), 28800.0), + // Health Connect's REAL shape. The oracle's `minute-dict` row is the throwing `{light,deep,...}` + // object; HC emits an ARRAY of `{stage,min}` objects, which parses cleanly and reaches the loop. + // It is the live producer shape this gate claims to be exempt from, so it is pinned rather than + // argued: every element is an object (no bail), none carries bounds (every segment skipped), + // covered stays 0, and zero cover is unmeasurable rather than bad. + Triple("hc-stage-min", """[{"stage":"light","min":300},{"stage":"deep","min":100}]""", 3600.0), + // A JSON null BOUND inside an otherwise well-formed object: the object itself is fine, so + // neither side bails; both fail to read the bound and skip the segment. + Triple("null-bound", """[{"start":null,"end":100,"stage":"deep"}]""", 1000.0), ) @Test @@ -126,4 +157,21 @@ class HypnogramCoverageTest { assertTrue(HypnogramCoverage.isHoled(seg(0 to 4200, 4200 to 8400), span)) assertEquals(8400.0 / span, HypnogramCoverage.fraction(seg(0 to 4200, 4200 to 8400), span)!!, 1e-12) } + + /** + * The ENGINE call site sums coverage off decoded segments, not off `stagesJson`, so the payload + * shape rule that exempts timestamp-free imports at the merge does not run there. What actually + * protects them is this: a group with no timestamped stages at all covers zero, and zero cover is + * unmeasurable rather than bad. Holds only while a group is single-sourced — a group mixing a + * staged fragment with a minute-dict one covers part of its total span and reads as holed. + */ + @Test fun groupWithNoTimestampedStagesIsUnmeasurableNotHoled() { + assertNull(HypnogramCoverage.fraction(0.0, 8 * 3600.0)) + } + + @Test fun mixedSourceGroupReadsAsHoled() { + // 8 h fully-staged fragment + a 2 h minute-dict fragment that decodes to no segments. + assertEquals(0.8, HypnogramCoverage.fraction(8 * 3600.0, 10 * 3600.0)!!, 1e-12) + assertTrue(0.8 < HypnogramCoverage.minCoverage) + } }