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) + } }