Repository navigation
Conversation
25986d1 to
c7de8a9
Compare
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
Thanks @kavemang — this is careful, and the parity work is the part I'd normally expect to find holes in. I checked it fairly hard and it holds up. One thing to change before merge, and two stale details.
Change before merge: Closes #1410 over-closes
The PR body says Closes #1410, so merging will close it. But #1410 proposes three tiers, and this implements one — which the code itself says out loud, on both platforms:
v33 -> v34 (#1410 tier 3)
v40 (#1410 tier 3)
The issue is explicit that they are not interchangeable: "(2) plus (1) is the cheapest pair that makes historical attribution possible at all; (3) is what makes it exact." Tiers 1 and 2 are the backup/export-level stamps, and nothing here touches a backup path — no file in the diff does. I confirmed neither BackupSettings carries a version today.
That matters beyond bookkeeping, because #746 is cited as needing exactly the tier-1 half: routing a foreign backup by what it states rather than by probing which tables it happens to carry. Closing #1410 on this would bury that.
Suggest dropping to a plain reference (Part of #1410, tier 3). If you'd rather close it and open a follow-up for tiers 1–2, that works too — the point is that it should be a decision rather than a side effect of the keyword.
Two stale details in the description
- Schema numbers. It says "Room schema 33 and GRDB migration v39". The actual migrations are Room 33 → 34 and GRDB v40 — #1586 took the numbers underneath you when it landed first.
- The stacked-PR warning at the top is now obsolete: #1586 is merged and this branch carries a single commit, so the dependency has already dropped out.
main...HEADis clean.
What I verified, rather than assumed
The parity surface is where this could have gone wrong quietly, so I went through it column by column:
- Schema is exactly identical. Same table, the same six columns in the same order and types, same composite PK
(deviceId, day, key), same index. - Room column order matches the entity field order — the trap CLAUDE.md calls out by name.
computedAtis milliseconds on both —System.currentTimeMillis()andInt64((timeIntervalSince1970 * 1_000).rounded()).computedByformats identically —"$platform:$appVersion+$appBuild"against"\(platform):\(appVersion)+\(appBuild)".- Scope storage strings match —
"score-window"/"metric-series"on both. Worth calling out: had these diverged, a cross-platform restore would have mis-scoped and the delete guard below would have silently stopped matching. - The delete guard is symmetric and correctly scoped. Both platforms filter
AND scope = ?withscoreWindow, so a daily window replacement cannot erase independently written weekly or metric-series stamps. That is the data-loss trap in this design and it is handled. - Schema oracle is byte-identical across the two copies (45,930 bytes) and valid JSON.
- Device deletion and identity adoption both cover the new table — Kotlin via explicit DAO methods, Swift via the table-sweep list. Different shapes, same coverage.
- Migrations are additive, registered in order, and don't mutate existing ones.
- The JaCoCo budget was not raised — only the regexes for renamed call sites. I checked specifically, because that budget is genuinely tight right now and raising it would have been the easy way out.
- Tests are real on both sides: Android pins the migration's additivity and start/end versions; Swift pins table, PK and index shape plus a round-trip including the nil-when-unstamped case.
One note, not a request
Leaving pre-migration history as absent-and-unknown rather than backfilling a guess is the right call, and the doc comments say so in both places. Worth keeping that wording if this gets revised — it is the kind of honest-unknown that tends to get "helpfully" filled in later.
Fix the closing keyword and I'm happy to merge. CI is green across all 14 checks, including test (StrandAnalytics), so the Swift half you couldn't build locally is covered.
…analyzeRecentOnCpu (#1696) analyzeRecentOnCpu had SEVENTEEN bytes of headroom: 61,518 against the 61,535-byte JaCoCo budget its own guard pins. Not a margin, a wall - and one that announces itself at test time rather than at review time, so the next person to touch that method for any reason hit it with no hint it had been full since before they arrived. analyzeRecentOnCpu instrumented Code length 61,518 -> 54,087 margin to the JVM's 65,535 method limit 4,017 -> 11,448 7,431 bytes freed by lifting the per-day skin-temp, SpO2 and off-wrist reads into a private suspend helper. IT IS A MOVE, AND THE DIFF PROVES IT. All 44 removed lines reappear verbatim in the additions; the extra lines are only the signature, the holder, the doc and the call site re-binding the same six names it used inline. Side-effect order is unchanged - the four per-owner memo maps are passed by reference and mutated in the same sequence, so WHEN the WHOOP 4.0 ADC anchor is learned and WHAT FROM are identical. The call sits inside the same prep timing bracket, so the analyzeRecent cost split still attributes these reads exactly as before. WHY EXTRACT RATHER THAN RAISE THE BUDGET. The guard exists because JaCoCo instrumentation already blew the JVM method limit once (bhelm#102), and this file already answered the question: persistFitnessVitalityAndSteps was extracted for exactly this reason, with a second test pinning that it stays extracted and at its required call site. Raising a budget to fit a change is how the method got to 17 bytes. WHY THIS BLOCK. Best ratio of bytes freed to boundary crossed - six values out against eleven in, all already local - and self-contained. The full 115-line read region was the blunt alternative: sixteen values crossing instead of six, for a refactor whose only job is to make room. THE BUDGET IS NOW A RATCHET, 61,535 -> 56,000. Leaving it would have left 7,448 bytes to be spent the same silent way, which is the mechanism this removes rather than merely resets. The ~1.9 K margin is deliberate: enough for an ordinary change (the pass-1 sliding read windows need about 825), small enough that a large regression fails at the test rather than at the 64 KB wall. 56,000 rather than the 55,000 first floated, because 55,000 would have left 88 bytes after the wiring and rebuilt the wall inside one PR. The comment states the discipline: lower it whenever an extraction frees space, never raise it to fit a change. persistFitnessVitalityAndSteps is left at 12,000 against 6,415 - slack, but not creeping, and tightening a method nobody is pressing against would be tightening for its own sake. MARKED DELIBERATELY ONE-SIDED. The Swift engine keeps this block inline, because the constraint is a JVM one and Swift has neither the limit nor the guard. The doc says do not mirror it, so a parity audit that finds a Kotlin helper with no Swift counterpart has found the intended state rather than a gap. REGRESSION SURFACE, CHECKED RATHER THAN ASSERTED. The three open PRs touching this file (#1605, #1587, #1572) conflict identically against main and against this branch - 44, 4 and 8 files, with IntelligenceEngine.kt already among #1605's against main. This adds no conflict. Nor can the ratchet block them: all three pass CI today, which is itself proof they add nothing to analyzeRecentOnCpu, since any addition would already fail at 17 bytes. They go from 17 bytes of room to 1,913. Android 4604 tests 0 failures, the same count as main. doc_comment_lint OK, i18n green. No Swift touched, so no app-build exposure.
|
Addressed the review: the PR now references #1410 as tier 3 without closing it, the stale stacking note is gone, and the migration details are current after rebasing (Room 40→41, GRDB v47). I also resolved the upstream persistence refactor/migration conflicts and reran the focused Android checks. |
9a82b76 to
19eca91
Compare
7cca6e7 to
452ab5d
Compare
…-provenance-1410 # Conflicts: # Strand/Data/IntelligenceEngine.swift
|
Thanks @kavemang - the schema work here is solid. All seven pins agree (Room 42, Two things I would like tightened before this goes in. 1. Kotlin stamps foreign-owner cells under the window's own
|
Reconcile typed parity declarations against their actual source counterparts, keep migration declarations in order, and regenerate derived authority. Refs #1410
2 participants
Part of #1410 (tier 3).
Summary
scoreComputationProvenancetable keyed by(deviceId, day, key).computedByandcomputedAtwith daily and metric-series computed values.platform:version+build, so restored cross-platform backups and builds sharing a marketing version remain distinguishable.Review follow-up
The Android scoring window now stamps only metric points belonging to its own computed device. Additional-owner cycle markers retain their actual stored owner and receive no stamp from the active window, matching Swift. Standalone metric-series batches group computation stamps by each row's device instead of using the first device for every cell.
Regression tests exercise the production DAO bodies for foreign-owner markers, mixed-device series and empty batches. The Swift marker-namespace test also asserts neither owner receives a window stamp for an additional-owner point. An unfiltered core Tools test reads both platforms' actual writer sources and pins disjoint score-window/standalone key sets, including
fitness_age.Current validation
At
0e5be16753556b9be682d0683422f441d6252ad8:Remaining blocker
GitHub reports
mergeable=false,mergeable_state=dirty, with zero checks at the current head. A merge preview identifies conflicts in bothTools/parity_twin_map.jsonand the typedTools/parity_dispositions.jsonauthority registry. The latter is reviewed input, rather than generated output. It needs reviewed reconciliation with current main before derived parity metadata can be regenerated and the complete current-head CI roster can run. A generated-only conflict repair cannot settle it. No rebase, force push or product conflict resolution was performed.Full Android/Room migration and schema validation, native Apple app builds and the complete CI roster remain pending at this head. Earlier validation in the review thread belongs to the previous head; it does not validate this follow-up. The schema/migrations themselves are unchanged by this follow-up.