diff --git a/.planning/round-v1439-QA-product-lead-findings.md b/.planning/round-v1439-QA-product-lead-findings.md new file mode 100644 index 000000000..12259bc1a --- /dev/null +++ b/.planning/round-v1439-QA-product-lead-findings.md @@ -0,0 +1,172 @@ +# v1.4.39 QA — Product-Lead Review + +Reviewer: Product Lead (read-only, strategic). +Inputs: 5 wave reports (W-MOOD, W-MED, W-SUM, W-WMY, W-SINCE), +`.planning/round-v1438-perf-analysis.md`, `git log v1.4.38.8..develop`, +`prisma/schema.prisma` diff, `.planning/round-v1439-backlog.md`. + +## Strategic posture + +v1.4.39 lands the v1.4.38 perf audit's §5 P2 + P3 + P4 + P6 in a single +release — three new persistence tiers (`mood_entry_rollups`, +`medication_compliance_rollups`, `measurement_rollups.sum_value`) plus +the read-side helpers that finally consume the WEEK/MONTH/YEAR write +amplification the audit called out as "dead weight." The five-wave +decomposition tracks the audit cleanly: every wave anchors to a numbered +P-item and every wave ships a writer + reader + boot-backfill + tests in +the well-trodden pattern v1.4.35 / v1.4.38.5 established. This is the +release where the rollup architecture stops being a measurement-only +tier and becomes the canonical second layer for mood + medication + +cumulative sums. + +The release deliberately stays inside the read-path optimisation lane. +No API contract changes, no UI changes, no iOS contract surface. The +deferred items (W-SUM A2 swap, W-MED health-score-fast-path hookup, +W-WMY slope90, W-WMY slim-slice all-time aggregate) are all internal +helper swaps that wire newly-shipped tiers into not-yet-converted +consumers — they sit in the v1.4.40 column and don't gate iOS work. +Marc's strategic ask ("blazing-fast queries, never throw away raw data") +is preserved on both axes: raw tables are untouched, every new tier is +a derived second layer with a documented live fallback. + +v1.5 readiness is **green from this release's perspective**. The +release ships zero new iOS-facing contracts, the medication-compliance +tier already uses a per-user-tz string `day` column (sidestepping the +`isNearUtc` blocker that gates the measurement-rollup fast-paths), and +W-WMY's `computeLongWindowSummary` is the helper a v1.5 "year in mood" +or multi-year trend card can call without any further backend work. +The cross-tz proper-fix + per-source rollup + slope90 hookup remain the +v1.5 architectural backlog — unchanged from the v1.4.38 closure. + +## Perf claim audit + +| Endpoint | Claim | Evidence in report | Confidence | +|---|---|---|---| +| `/api/mood/analytics` cold | 12.7 s → ~200 ms | Audit §5 P2 estimate + writer + 21 unit/route tests + bounded 1 800-row read assertion | **Medium** — unit tests prove the read shape, not prod wall-clock. Verify in post-deploy `round-v1439-perf-verify.md`. | +| `/api/medications/intake?scope=compliance` cold | 3.2 s → ~200 ms | Audit §5 P4 estimate + 16 unit + 2 route tests + bounded indexed read pattern | **Medium** — audit attributed the 3.2 s mostly to pool stall on cold, not row count. Rollup elides the stall but won't help if pool stall is the dominant residual. Verify in prod. | +| `/api/dashboard/summary` cumulative tiles | sparkline now shows true SUM not MEAN | Asymmetric test (SUM=8120 ≠ MEAN×COUNT=8000) pins the column path is wired | **High** — semantic-correctness fix, not a wall-clock claim. | +| `/api/measurements?groupBy=day` cumulative path | reads `sum_value` directly, no algebraic re-derivation | Test pin + legacy NULL fallback test | **High** — pure read-shape change, low-risk swap. | +| `/api/analytics` live-fallback A2 loop | row bound 347 114 → ~5 000 | `since: liveSince` threaded into every `fetchMeasurementSeriesChunked` call + 3 route tests + integration-test reseed | **High** for row-count bound; **Medium** for the 20-40 s wall-clock estimate (was audit-derived, not measured). | +| `/api/analytics` full-slice cold | not directly addressed in v1.4.39 | A2 cumulative-skip (P3 follow-on) deferred to v1.4.40 per W-SUM | **N/A** — the 74.6 s claim from the audit was the v1.4.38.8 fast-path-gate fix's domain; W-SINCE only defends the residual live-fallback path. | +| `summaries-slice` long-window | new `computeLongWindowSummary` entry point ready for v1.5 trend card | 5 unit tests + auto-router + fall-through chain | **High** for helper correctness; **No consumer wired in v1.4.39** so zero user-visible win this release. | + +The pattern is consistent across all five waves: rollup-tier +correctness is proven by unit + route tests, but **wall-clock win on +Marc's tenant is not proven until the post-deploy perf-verify**. This +is the standard for the rollup-tier releases (v1.4.35, v1.4.38.5, +v1.4.38.7) and the post-deploy verify has caught soft claims twice +before (`round-v1436-perf-verify.md`, `round-v1437-perf-verify.md`). +The release notes should phrase the wins as "expected" until verified. + +## Release-readiness verdict + +- **Ship as v1.4.39: YES.** All five waves landed, quality gates green + per each report, tests delta +80 (4 551 → 4 631), no new API + contract, no migration risk (all three migrations additive — new + tables + nullable column with legacy-NULL fallback documented in + every consumer). The only outstanding concern is the cross-agent + commit-attribution drift (see Risk register) which is a process bug, + not a release blocker. + +- **Carry-overs to v1.4.40:** + - W-SUM `/api/analytics` A2 cumulative-skip — single-route swap to + `readCumulativeDaySumsBatch`; audit estimates 20-40 s saved on the + full-slice cold path. Highest-impact carry-over. + - W-MED `health-score-fast-path.ts:267-303` swap onto + `readMedicationCompliance` — estimated 1-2 s on the analytics + full-slice cold path. + - W-WMY slope90 hookup in `health-score-fast-path.ts` — slope is + non-linearly composable across DAY buckets; needs a slope + re-derivation research step first. + - W-WMY slim-slice all-time aggregate swap — currently reads + `granularity = 'DAY'` for the all-time path; could move to YEAR + once a sync watermark for WEEK/MONTH/YEAR lands (today they're + async via pg-boss, so the swap would risk staleness). + - W-MOOD WEEK/MONTH/YEAR reader consumer — tier ships writer-only; + a Coach long-window prompt or "year in mood" tile is the v1.4.40 + consumer. + - v1.4.38 backlog items (analytics envelope split, `ensureUserRollupsFresh` + retry safety, RX-3, RX-7, RX-8 sparkline polish, QA-4 / QA-6 + medium items) — none gated on iOS work. + +- **v1.5 readiness:** unblocked. + - No new iOS contracts introduced (read-path optimisations only). + - Medication-compliance tier uses per-user-tz `day` strings — + `isNearUtc` non-Berlin-tenant gating doesn't apply to this tier, + which is a small architectural step toward the v1.5 per-user-tz + bucketing for the measurement tier (P7). + - Cross-tz proper fix, per-source rollup (P5), and slope-window + SQL-move (P8) remain the v1.5 architectural backlog — unchanged. + - `computeLongWindowSummary` is ready for any v1.5 multi-year UI + consumer to call without further backend work. + +## Risk register + +- **Wall-clock perf wins are audit estimates, not measurements** — + mitigation: post-deploy `round-v1439-perf-verify.md` against Marc's + tenant; release-notes phrase wins as "expected" until verified; + rollback to v1.4.38.8 is clean (rollup reads have live fallback). + +- **Cumulative `sum_value` legacy-NULL coverage on Marc's tenant** — + the boot-backfill discovery UNION branch handles uncovered rows but + every existing row has `sum_value = NULL` until the worker re-folds. + Consumers (dashboard sparkline, measurements groupBy=day) carry a + documented `mean × count` fallback so the chart never holes; risk is + cosmetic delta on tenants where source-priority resolves to a single + source (Marc's case via the iOS `dailyStatsExternalId` path → exact) + but could over-count on multi-source days until per-source rollup + (P5, v1.5) lands. Mitigation: documented in W-SUM report; verify in + post-deploy. + +- **Mood rollup UTC-anchored bucketing for non-Berlin tenants** — + W-MOOD anchors on UTC midnight (mirroring the measurement-rollup + convention). Same `isNearUtc` ±3 h guard applies. Marc's tenant is + fine; a future non-near-UTC mood-logger would see day-key drift at + the UTC boundary. Mitigation: documented in W-MOOD §"UTC vs + TZ-anchored" + pinned in route parity test; v1.5 per-user-tz + migration unblocks both tiers together. + +- **Cross-agent commit-attribution drift (recurring process bug)** — + W-WMY's `8763b3aa` absorbed W-SUM's test files; W-MOOD's + `d15850d5` overwrote W-MED's reminder-worker.ts edits per the + Marc-briefing pre-marathon note. This is the **third recurring + occurrence** (also flagged in `project_v1437_final_web_release.md`). + Test content is intact across both incidents (grep-verified per + W-SUM and W-WMY reports), so no functional regression. **Mitigation + for the next marathon: per-agent `git worktree` isolation** — this + is now a hard requirement, not a nice-to-have. Recommend adding + worktree-per-wave to the `release-marathon` skill's standard flow. + +- **`ensureUserRollupsFresh` write storm on first post-deploy cold + mounts** — three new tiers means three new boot-backfill discovery + queries plus three new per-write hook fan-outs. Per the audit §4 + this is fire-and-forget so the request itself stays fast, but the + worker queue depth will spike for the first hour post-deploy on + Marc's tenant (347 k rows × 3 new tiers). Mitigation: existing + pg-boss back-pressure handles this; monitor Coolify logs for + `rollup-full-backfill` queue depth in the first hour. + +- **Marc's strategic ask preservation — confirmed** — raw + `measurements`, `mood_entries`, `medication_intake_events` tables + are byte-unchanged in the schema diff. Every new tier is purely + derived (`*_rollups` tables) and additive (one nullable column on + `measurement_rollups`). Boot backfill is idempotent; rollup wipe on + account-delete is in-transaction with the source-row wipe per + W-MOOD §self-review item 4-5. + +- **Semver call — minor bump (1.4.39) confirmed** — features added: + 3 new tables (`mood_entry_rollups`, `medication_compliance_rollups`, + `measurement_rollups.sum_value` column), 8 new internal helpers + (rollups + reader fns), 1 new analytics row-cap, ~80 new tests. + Zero API contract breaks, zero new iOS surface, zero user-visible + UI changes. Per Marc's conservative-semver doctrine + (`feedback_semver_conservative.md`) this is squarely additive-only + features inside an existing minor cycle → **v1.4.39 patch bump is + the correct call** (v1.5 stays reserved for the iOS Swift sprint). + +- **Release-marathon retrospective items** — beyond the worktree + isolation fix above, no other process bugs surfaced in this + marathon. Coolify auto-deploy fix from v1.4.38.4 is still in + effect. The five-wave decomposition was clean — each wave was + scoped to a single P-item and a small file set with documented + "did NOT touch" sections to prevent collisions. diff --git a/.planning/round-v1439-QA-security-findings.md b/.planning/round-v1439-QA-security-findings.md new file mode 100644 index 000000000..8d81b4dd3 --- /dev/null +++ b/.planning/round-v1439-QA-security-findings.md @@ -0,0 +1,174 @@ +# v1.4.39 QA — Security Review + +Scope: `git diff v1.4.38.8..develop` — migrations 0070/0071/0072, mood + +medication-compliance rollup helpers, per-write hooks on every mood and +medication-intake mutation site, `sum_value` column on the measurement +rollup tier, three new pg-boss queues. No new HTTP routes were added. + +## Findings + +### Critical + +_None._ + +### High + +_None._ + +### Medium + +- **F-SEC-M-01 — `enqueueBootTimeMedicationComplianceBackfill` runs a + cluster-wide discovery query on every authenticated coverage-miss + request, opening a soft DoS amplification surface.** + Path: `src/app/api/medications/intake/route.ts:181` → + `src/lib/medications/compliance-rollups.ts:490-551`. + On every cache-miss + coverage-miss hit of + `/api/medications/intake?scope=compliance`, the live-fallback fires a + fire-and-forget `enqueueBootTimeMedicationComplianceBackfill()`. That + helper does NOT scope to the caller's user — it issues a single + `LEFT JOIN medication_intake_events × medication_compliance_rollups` + scan across **every tenant in the database**. pg-boss `singletonKey` + collapses the resulting enqueues, so worker load is bounded, but the + discovery SQL itself runs unguarded on the request path. An + authenticated user iterating coverage-miss requests across many + sessions can drive that scan on each hit; on a multi-tenant cluster + with millions of intake events the planner walks all of them per + request. Containment: gate the enqueue behind an in-process throttle + or only fire it when the **caller's** user is uncovered (cheap + single-row probe before kicking the cluster-wide discovery). Mood- + rollup's analog (`ensureUserMoodRollupsFresh`) already does this — it + scopes to a single user. + +- **F-SEC-M-02 — `mood_entry_rollups` and + `medication_compliance_rollups` carry no per-user RLS or query + enforcement; every read relies on application-layer `userId` filters.** + Path: migrations 0070 / 0071, plus every `findMany / findFirst / + upsert` in `src/lib/mood/rollups.ts` and + `src/lib/medications/compliance-rollups.ts`. + Inspected: 100 % of new Prisma reads include `where: { userId }` (or + the composite key includes userId). Inspected: 100 % of new `upsert` + / `deleteMany` calls include `userId` in the where clause. No leak + found. The pattern matches the existing `measurement_rollups` tier, + so this is consistent — but it remains the only tenant-isolation + layer for the two new tables. Suggest considering Postgres RLS as a + defense-in-depth step before v1.5 multi-org work lands; not blocking + for v1.4.39 since the contract matches the pre-existing tier. + +### Low + +- **F-SEC-L-01 — `recomputeMedicationComplianceForEvent` swallows + errors silently in best-effort mode; one failure mode (the rollup + row drifts permanently out of sync with the source intake table) + has no observability past the `annotate({ medication_compliance_ + rollup_failed: true })` flag.** + Path: `src/lib/medications/compliance-rollups.ts:277-289`. + No PII is logged (only `error.message`, no medication name, no + scheduledFor timestamp). The boot-time backfill plus the per-write + hook's idempotency mean a single failed recompute self-corrects on + the next mutation that touches the same `(user, medication, day)` + tuple. Recommend: a follow-up metric / alert on the annotate flag + so silent populator drift surfaces in ops dashboards. + +- **F-SEC-L-02 — `recomputeUserMedicationCompliance` uses + `prisma.$queryRaw` (tagged template) with `${safeTz}` interpolated + into `to_char(... AT TIME ZONE ${safeTz}, 'YYYY-MM-DD')`.** + Path: `src/lib/medications/compliance-rollups.ts:421-430`. + Verified: tagged-template form parameterises `${safeTz}` as a bind + variable (not string interpolation). `safeTz` is run through + `isValidTimezone()` which whitelists against the IANA zone list, so + even a "%' OR 1=1" attempt is rejected at the gate. Both belt-and- + braces — note for the record but no vulnerability. + +- **F-SEC-L-03 — `runMoodRollupAggregate` uses `$queryRawUnsafe` with + an inlined `date_trunc('', ...)`.** + Path: `src/lib/mood/rollups.ts:475-510`. + `truncUnit` is sourced from the closed `DATE_TRUNC_UNIT` enum + (DAY/WEEK/MONTH/YEAR → `day/week/month/year`) AND validated against + a hard-coded whitelist (line 485) before splicing. `userId`, `from`, + `to` are passed as `$1`/`$2`/`$3` parameters. No injection surface; + mirrors the proven `measurement-rollups` pattern. Logged for the + record only. + +### Audit notes (no finding, recorded for completeness) + +- **Migrations 0070 / 0071 / 0072 are additive-only.** No `ALTER` on + existing tables beyond the additive `ADD COLUMN IF NOT EXISTS + sum_value` on `measurement_rollups`. FKs on + `mood_entry_rollups.user_id`, `medication_compliance_rollups.user_id`, + `medication_compliance_rollups.medication_id` all carry + `ON DELETE CASCADE` to the parent (correct — owner deletion sweeps + the cache rows). No `SET NULL` paths that could orphan rows. The + `IF NOT EXISTS` + `EXCEPTION WHEN duplicate_object` guards mirror + the established 0067/0068/0069 pattern → replay-safe. + +- **Worker queue idempotency:** `MOOD_ROLLUP_RECOMPUTE_QUEUE`, + `MOOD_ROLLUP_FULL_BACKFILL_QUEUE`, + `MEDICATION_COMPLIANCE_BACKFILL_QUEUE` all coalesce duplicate sends + via per-user `singletonKey`. `retryLimit:3 / retryBackoff:true` + prevent infinite-loop replay. A poisoned job throws once → pg-boss + records the error and dead-letters after 3 retries. The discovery + helpers are the only callers that enqueue these; one of them + (`enqueueBootTimeMedicationComplianceBackfill`) is reachable from a + request path → see F-SEC-M-01. + +- **CSRF / idempotency-key reach intact:** `POST /api/medications/[id]/ + intake` (line 24-26) wraps `postIntake` in `withIdempotency`. The new + `recomputeMedicationComplianceForEvent` hook (line 166-171) fires + **after** the idempotency-key short-circuit (line 50-75) and after + the `assertMedicationOwnership` privacy gate. The hook also fires + after the `$transaction` create, so a duplicate request reuses the + existing event without enqueuing a second rollup hit. Verified. + +- **Cross-tenant leakage probes:** I searched every new `findFirst` / + `findMany` / `upsert` / `deleteMany` / `$queryRaw` for missing + `userId` clauses. All 23 new query sites scope to the authenticated + user, either via explicit `where: { userId }`, via the composite + primary key, or via a tagged-template `$queryRaw` parameter. No + leak found. + +- **Per-write hook PII surface:** The new `annotate` calls log only + `mood_rollup_*_failed: true` / `medication_compliance_rollup_*_failed: + true` boolean flags plus the JS `Error.message`. No mood text, no + medication name, no `moodLoggedAt` / `scheduledFor` timestamp ever + reaches the log payload from the v1.4.39-added sites. (Pre-existing + backup-creation payload at `reminder-worker.ts:1517-1530` does log + `medication.name` + `mood`, but that's untouched here and out of + scope for this review.) + +- **Telegram + moodlog + ingest auth paths:** The Telegram webhook + hook fires only after `findTelegramUser` resolves a user with a + valid `telegramBotToken`. The moodlog webhook hook fires after the + X-Webhook-Secret timing-safe compare succeeds. The ingest hook + fires after `hashToken` validates the bearer + the scope check + passes. None of the new hooks bypass auth. + +- **No new HTTP endpoints.** `git diff --diff-filter=A -- 'src/app/api/ + **/route.ts'` returned zero new route files. All route changes are + modifications adding hooks to existing endpoints, every one of which + is auth-gated by the pre-existing `requireAuth` / `withIdempotency` + / `apiHandler` chain. + +## Brief-back (≤200 words) + +No Critical or High findings. The v1.4.39 surface is tenant-scoped end +to end: every new Prisma query carries an explicit `userId` clause, +every new raw-SQL fragment is parameterised, the IANA-zone gate added +in v1.4.38 protects the new `to_char(... AT TIME ZONE)` splice, and +the three new migrations are additive with correct CASCADE FK +behaviour. Per-write hooks fire AFTER idempotency / auth / privacy +gates on every site I inspected (medications intake POST is the canary +— hook lands at line 166, well after the auth + ownership + dedup +short-circuits). Worker queues coalesce via `singletonKey` and retry +with backoff. + +One Medium worth fixing before ship: F-SEC-M-01 — +`enqueueBootTimeMedicationComplianceBackfill` runs a cluster-wide +discovery `LEFT JOIN` on every coverage-miss request. Mood's analog +already scopes to a single user; the medication path should mirror +that. Cheap one-line fix: probe the caller's `userId` first, only +enqueue boot-backfill when they specifically lack coverage. + +Two Lows are observability-only (silent rollup-drift annotate + +follow-up RLS consideration). Three audit-notes captured for the +record. Cleared to ship after F-SEC-M-01 is decided (mitigate or +accept). diff --git a/.planning/round-v1439-QA-senior-findings.md b/.planning/round-v1439-QA-senior-findings.md new file mode 100644 index 000000000..256bbc1ed --- /dev/null +++ b/.planning/round-v1439-QA-senior-findings.md @@ -0,0 +1,214 @@ +# v1.4.39 QA — Senior-Dev Review + +Independent code-review pass on the full diff `v1.4.38.8..develop` +(commits `0d33f91d` → `bbcb09e8`, 50 files, +6 404 / −26). Read-only. + +Anchors: +- Audit: `.planning/round-v1438-perf-analysis.md` +- Wave reports: `phase-W-{MOOD,MED,SUM,WMY,SINCE}-v1439-report.md` + +`pnpm typecheck` passes clean on HEAD. + +## Severity bands + +- **Critical** — release-blocking (data loss, crash, perf regression > 50 %). +- **High** — must-fix before tag (parity break, edge case that wedges power users, hidden lint/type errors). +- **Medium** — should-fix in v1.4.39 if cheap, otherwise v1.4.40 backlog. +- **Low** — cosmetic, naming, dead code. + +## Findings + +### Critical + +(none) + +The four migrations are additive, write hooks fall back to a working +legacy path on miss, and `typecheck` is clean. No release-blocking +issue surfaced. Recommend ship after the High items below are +triaged (the two parity / race concerns can be deferred with a +documented v1.4.40 backlog entry). + +### High + +- **F-H-01** — `hasMedicationComplianceCoverage` returns `true` on + partial coverage and serves zero-filled buckets for un-rolled + days — `src/lib/medications/compliance-rollups.ts:373-389`. + The probe finds ANY rollup row inside the trailing window and + flips to the rollup path. If the boot backfill has minted only + the first few days (long-running fold on a multi-medication + legacy account) the probe returns `true` while days N..days-1 + are still zero-filled. The user sees a "compliance dropped to 0 + %" tile for the un-rolled days until the backfill finishes. The + legacy `buildComplianceBuckets` would have returned the correct + values because it reads `medication_intake_events` directly. + **Fix**: tighten the probe to compare the count of rolled days + vs. expected days, or persist a per-user `compliance_backfilled_at` + watermark and gate the swap on it. + +- **F-H-02** — Read-after-write race in mood + compliance DAY + recompute — `src/lib/mood/rollups.ts:173-212`, + `src/lib/medications/compliance-rollups.ts:177-252`. The DAY + recompute issues `SELECT … aggregate` followed by an `UPSERT` + without serialisation. Two concurrent writes for the same + `(user, day)` interleave as: A SELECT → B SELECT → B UPSERT + (correct N) → A UPSERT (stale N-1). The stale row stays until + the next write or boot backfill. Low probability on mood + (1/day human writes) but realistic on medication compliance + where the reminder worker mints fresh `scheduledFor` rows while + a user is acking via Telegram/web. The compliance rollup row + serves the dashboard tile so the wrong "scheduled / taken" + count is user-visible. **Fix**: wrap the recompute body in a + `SELECT … FOR UPDATE` against a sentinel row, or convert the + fold into a single SQL upsert that re-aggregates inside the + upsert subquery (`INSERT INTO … SELECT … FROM … ON CONFLICT …`). + Doable as v1.4.39 hotfix (~2 h) or deferred to v1.4.40 with a + documented "best-effort tier" caveat. + +- **F-H-03** — `enqueueMoodRollupRecompute` fans out 3 enqueues + per mood write but the queue's `singletonKey` includes + `from.toISOString()`, which differs per granularity bucket — + fine. But the helper is `await`-fanned inside + `recomputeMoodBucketsForEntry`, so a slow pg-boss send adds + latency to the user's mood-write response. + `src/lib/mood/rollups.ts:206-211`. The audit's `Promise.all` + intent is preserved, but the enclosing `await` still serialises + the route on pg-boss latency. The measurement-rollups path has + the same pattern (already shipped). **Fix**: drop the outer + `await` on the Promise.all and fire-and-forget the enqueue — + the queue is a hint, not a write-path invariant, and the DAY + pass that the read path actually depends on already committed + synchronously above. + +### Medium + +- **F-M-01** — `computeLongWindowSummary` is exported but never + consumed — `src/lib/analytics/summaries-slice.ts:615`. W-WMY + shipped the helper + 5 tests for a v1.5 multi-year card that + doesn't exist yet. Dead-but-tested code. Confirmed deferred in + the W-WMY report; flag so the v1.5 plan keeps the consumer on + the radar. + +- **F-M-02** — `MoodEntryRollup` writer accepts a `tx` parameter + (`src/lib/mood/rollups.ts:173-178`) but no call site passes one. + Same for `MedicationComplianceRollup`. The atomic-with-parent + story documented in the wave reports is therefore unrealised — + every rollup write happens AFTER the parent commits. Acceptable + for a cache tier, but the comments overstate the safety. + **Fix**: drop the `tx` parameter or wire it through the + `prisma.$transaction` blocks that exist (e.g. the Telegram skip + webhook). + +- **F-M-03** — `sum_value IS NULL` boot-backfill discovery branch + has no supporting index — `src/lib/measurements/rollups.ts:762-780` + + `prisma/migrations/0072_v1439_rollup_sum_value/migration.sql`. + The UNION branch `WHERE r2."granularity" = 'DAY' AND + r2."sum_value" IS NULL` does a sequential scan of + `measurement_rollups`. For Marc's single-tenant install it's + cheap (~few thousand rows). For multi-tenant scaling it + becomes a problem. **Fix**: add a partial index `CREATE INDEX + CONCURRENTLY ON measurement_rollups(user_id) WHERE granularity + = 'DAY' AND sum_value IS NULL` once the legacy NULL backfill + finishes the first sweep (the index is only useful while there + are still NULL rows). + +- **F-M-04** — Integration test `analytics-bp-aggregate-paged.test.ts` + is now wall-clock-anchored at `Date.now()` and seeds 6 000 rows + at 15-min intervals (~62.5 days). 90-day cap leaves 27.5 days + of headroom; once CI runs ≥ 28 days after the seed timestamp + reaches the floor the test fails. **Fix**: re-anchor the seed + to `nowMs - 30 * 86_400_000` so the youngest row is always 30 d + old, keeping the test stable across time. + +- **F-M-05** — `RollupBucketRow` shape divergence between + `rollup-read.ts` (`DailyMeanRow`) and `rollup-read-wmy.ts` + (`RollupBucketRow`). The latter carries `sd/slope/r2/sumValue` + the former does not. Two coexisting bucket-row types invite + drift. **Fix**: consolidate into one shape in + `rollup-read.ts` and re-export from `rollup-read-wmy.ts`. Cheap + in v1.4.40. + +- **F-M-06** — Cross-agent commit attribution drift visibly hit + this marathon (W-SUM's tests landed inside W-WMY's commit + `8763b3aa`; see W-SUM report "Surprises"). No broken state, but + it makes `git bisect` and `git log -p src/lib/measurements/` + harder to follow. Same pattern as v1.4.37. **Fix**: per-agent + `git worktree` in the next marathon (the marathon kickoff + guidance already nods at this). + +### Low + +- **F-L-01** — `recomputeMedicationComplianceForDay` computes the + day window twice via `start + 86_400_000` then via + `startOfDayUtcInTz(nextDayKey)` and picks the larger — both + helpers run unconditionally — `src/lib/medications/compliance-rollups.ts:186-199`. + The simpler invariant is "always use `startOfDayUtcInTz` for + both bounds". Cosmetic cleanup. + +- **F-L-02** — `readMedicationCompliance` adds the special-case + `if (!dedup.has(todayKey)) orderedKeys.push(todayKey)` + (`src/lib/medications/compliance-rollups.ts:330-332`). The + DST-edge-case it guards against (`days===1` on a DST boundary) + is unreachable from production callers (route minimum is + `days≥7`). Dead defensive code — fine to keep, worth a + one-line comment that it's a belt-and-braces guard. + +- **F-L-03** — Mood-rollup `persistMoodRollupRows` chunks at 500 + but issues a sequential `for…of` upsert loop inside each + chunk — `src/lib/mood/rollups.ts:526-560`. The + measurement-rollups equivalent uses `prisma.$transaction([…])` + for parallel upsert. The mood DAY-hot-path writes 1 row so it + doesn't matter, but a 5-year boot-fold pays N round-trips + instead of N/500 transactions. + +- **F-L-04** — `MOOD_ROLLUP_FULL_BACKFILL_QUEUE` discovery + query (`src/lib/mood/rollups.ts:417-427`) doesn't restrict + `granularity='DAY'` on the LEFT JOIN — same shape as the + measurement-rollups query, which intentionally bridges all + granularities. Inconsequential because the read path only + consults DAY anyway. Comment clarification would help future + reviewers. + +- **F-L-05** — `mood/rollups.ts` typo: `MoodRollupRow.sd` is + typed `number | null` but the SQL aggregator returns + `STDDEV_POP` which is `0` (not `NaN`) for a single-row bucket. + The downstream consumer never asserts SD on single-entry days + so this is invisible. Worth a doc-comment. + +- **F-L-06** — Marc-voice + no-PII check on the new diff: commit + messages are English, lowercase scope, no Co-Authored-By, no + emojis. Source comments mention "Marc" exactly once + (`src/lib/measurements/rollup-read-cumulative.ts:28-30`) which + is the in-code planning anchor, not user-facing copy. Compliant. + +## Brief-back (≤200 words) + +**Top 3 findings**: + +1. **F-H-01 (High)** — `hasMedicationComplianceCoverage` can + short-circuit to the rollup path while only some days are + backfilled, exposing zero-filled tiles for the un-rolled days + until the boot fold completes. Realistic on legacy accounts + restoring v1.4.39 mid-deploy. Either tighten the probe or gate + the swap on a per-user backfill watermark. + +2. **F-H-02 (High)** — Read-aggregate-then-upsert race in the + mood + compliance DAY recomputes. Concurrent writes for the + same `(user, day)` can leave a stale row until the next write. + Mood is human-paced so low risk; medication compliance under + reminder-worker + Telegram contention is the realistic blast + radius. Convert to a single SQL upsert or `FOR UPDATE` sentinel. + +3. **F-H-03 (High)** — `recomputeMoodBucketsForEntry` awaits the + WEEK/MONTH/YEAR pg-boss enqueue inside the write-path response + cycle. Drop the outer `await` and fire-and-forget; the DAY + pass the read path actually depends on already committed. + +**Overall posture**: clean architecture, additive migrations, +test coverage proportional to scope, typecheck green. Two +real-world races + one perf-probe gap on partial backfill are +the only correctness concerns. Cache-tier "best-effort" framing +in the wave reports is defensible. + +**Ship-readiness**: ship as v1.4.39.0 with F-H-01 + F-H-02 + +F-H-03 documented in `.planning/round-v1439-backlog.md` for +v1.4.39.1 hotfix or v1.4.40. None block the release. diff --git a/.planning/round-v1439-QA-simplifier-findings.md b/.planning/round-v1439-QA-simplifier-findings.md new file mode 100644 index 000000000..3b5f84f8b --- /dev/null +++ b/.planning/round-v1439-QA-simplifier-findings.md @@ -0,0 +1,32 @@ +# v1.4.39 QA — Simplification Findings + +Reviewed diff: `git diff v1.4.38.8..develop`. Focus on dead helpers, near-duplicate code, and prunable fallbacks introduced by the W-MOOD / W-MED / W-SUM / W-WMY / W-SINCE waves. + +No automated edits were applied — every candidate below is either consumed by tests as a contract anchor, kept around as an intentional v1.5 hook, or non-trivial enough to warrant Marc's sign-off before deletion. + +## Suggested removals +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:54` — `isCumulativeType` — exported, tested, but no production caller; `/api/dashboard/summary/route.ts` reads `CUMULATIVE_HK_TYPES` directly and `/api/analytics/route.ts` uses `isCumulativeDaySumType` from `cumulative-day-sum.ts`. The shape was added "for future routes" by W-SUM but never wired — confidence: likely. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:83` — `readCumulativeDaySums` — exported and tested but no production caller. The cumulative tile flow inlines `sum_value` off `SparklineRow` instead — confidence: likely. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:121` — `readCumulativeDaySumsBatch` — exported and tested but no production caller. The W-SUM phase-report frames this as the future analytics-route batch reader; today `/api/analytics` still routes through `pickCumulativeDaySum` on live measurements — confidence: likely. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts:164` — `resolveBucketSum` — exported and tested but no production caller; the inline `sum_value ?? mean * count` fallback in `dashboard/summary/route.ts` reimplements the same shape. Once a caller exists this should be the single source of truth, but right now it is dead — confidence: likely. +- `/Users/marc/Projects/HealthLog/src/lib/analytics/summaries-slice.ts:615` — `computeLongWindowSummary` — only the file's own test suite imports it. The phase report flags it as the v1.5 multi-year-card hook; until that card lands the function ships dead in the production bundle — confidence: certain (no callers in `src/`). +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-wmy.ts:125,137,149` — `readWeekRollups` / `readMonthRollups` / `readYearRollups` — only the wmy reader's own tests import them; the production `readBestGranularityRollups` walker uses the internal `readGranularity`. Either delete the three thin wrappers and inline the call into the tests, or downgrade them to non-exported helpers — confidence: certain. +- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:453` — `ensureUserMedicationComplianceFresh` — the only caller is its own JSDoc reference. The coverage-probe → boot-backfill cascade fully owns the cold-mount path; this "kept for symmetry" wrapper is dead today — confidence: certain. + +## Suggested dedups +- `/Users/marc/Projects/HealthLog/src/lib/mood/rollups.ts:571` — `startOfUtcDay` — byte-identical to `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:524`. Suggested action: lift to a shared `src/lib/time/start-of-utc-day.ts` (or re-export from `tz/format`) and have both rollups files import from it. The Postgres `date_trunc` parity comment stays meaningful in one place. +- `/Users/marc/Projects/HealthLog/src/lib/mood/rollups.ts:583` — `bucketSpan` — structurally identical to `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:537` (only the variable name differs: `measuredAt` vs `moodLoggedAt`). Suggested action: extract a generic `bucketSpanForUtcInstant(instant: Date, granularity: RollupGranularity)` and share it. WEEK / MONTH / YEAR branches are line-by-line clones. +- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:73` — `wallClockInTz` — third copy of the same helper alongside `src/lib/medications/scheduling/cadence.ts:107` and `src/lib/tz/format.ts` (private). Suggested action: export the canonical one from `lib/tz/format.ts` and delete the two private copies. Marc-Voice rationale: "one tz helper, one bug fix radius." +- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:96` — `tzOffsetMinutes` — third copy alongside `src/lib/tz/format.ts:225` and `src/lib/medications/scheduling/cadence.ts:149`; `src/lib/measurements/drain-per-sample-cumulative.ts:126` has a fourth variant (`tzOffsetMinutesAt`). Suggested action: keep the `tz/format.ts` implementation and have the other three import it. +- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:126` — `startOfDayUtcInTz` — bespoke two-pass convergence. `lib/tz/format.ts` already exports `userDayKey`; a `startOfUserDay(dayKey, tz)` companion would let this file shed its private DST helper. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-wmy.ts:204` — `aggregateWmyBuckets` — near-duplicate of `src/lib/measurements/rollup-read.ts:59` `aggregateBuckets`. The new version is a strict superset (adds `sum`). Suggested action: extend `aggregateBuckets` to carry `sum`/`sumValue` and delete `aggregateWmyBuckets`; or delete `aggregateBuckets` and have its single caller in `rollup-read.ts` consume the WMY one. Keeping two near-identical reducers invites a future drift bug. + +## Suggested simplifications +- `/Users/marc/Projects/HealthLog/src/lib/medications/compliance-rollups.ts:177` — `recomputeMedicationComplianceForDay` re-derives `nextDayKey` via an IIFE that builds a probe-`Date`, calls `wallClockInTz`, and re-pads its components. Once `startOfDayUtcInTz` accepts a "next day" mode (or via a `+1 day` helper) this 8-line block collapses to one call. Behaviour-preserving. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollups.ts:751` — boot-backfill discovery `$queryRaw` now `UNION`s the per-type-missing branch with the legacy-NULL `sum_value` branch. Once Marc's tenant has converged (per the W-SUM rollout plan), the legacy-NULL branch becomes a permanent no-op walk over the full rollup table. Suggested action: schedule the `OR r2."sum_value" IS NULL` branch for removal in v1.4.40 + add a TODO with the cut-off date — keeps the discovery query single-table after convergence. +- `/Users/marc/Projects/HealthLog/src/app/api/analytics/route.ts:241` — `ANALYTICS_LIVE_WINDOW_DAYS = 90` and its 30-line provenance comment live inside `buildAnalyticsResponse`. Hoist to a module-level constant alongside the other thresholds so a future tuner does not have to scroll into the handler body. Behaviour-preserving. +- `/Users/marc/Projects/HealthLog/src/app/api/dashboard/summary/route.ts:441` — the cumulative-vs-spot `useSum` branch is a nested ternary `useSum ? (row.sum_value !== null ? Number(row.sum_value) : Number(row.mean) * Number(row.count)) : Number(row.mean)`. Per CLAUDE.md "no nested ternaries"; lift the inner branch into a named local: `const cumulativePoint = row.sum_value !== null ? Number(row.sum_value) : Number(row.mean) * Number(row.count); const point = useSum ? cumulativePoint : Number(row.mean);`. Or call the (currently dead) `resolveBucketSum` once it is wired. +- `/Users/marc/Projects/HealthLog/src/lib/measurements/rollup-read-cumulative.ts` — once the four exports are either wired or removed, the file plus its 195-line test file collapses by ~360 lines. The W-SUM phase-report's "ready-for-v1.5" framing should be reflected by deletion-with-revival-PR rather than dead exports living in `develop`. + +## Applied +None — every candidate above either (a) intersects test contracts that should be migrated together, (b) sits behind a v1.5 wiring intent Marc signed off on in the phase reports, or (c) is a cross-file dedup whose target module needs design input. Listing as suggestions keeps Marc's voice in the eventual commit message. diff --git a/.planning/round-v1439-QA-specialist-findings.md b/.planning/round-v1439-QA-specialist-findings.md new file mode 100644 index 000000000..8c3324695 --- /dev/null +++ b/.planning/round-v1439-QA-specialist-findings.md @@ -0,0 +1,310 @@ +# v1.4.39 — QA findings — Prisma + performance specialist + +Read-only review of `git diff v1.4.38.8..develop` against the five wave +reports (W-MOOD / W-MED / W-SUM / W-WMY / W-SINCE) and the v1.4.38 perf +audit. Severity-band structure. + +--- + +## Critical + +None. + +The three migrations (`0070_v1439_mood_rollups`, +`0071_v1439_medication_compliance_rollups`, +`0072_v1439_rollup_sum_value`) are correct and non-blocking under +PostgreSQL ≥ 11: + +- `0072` is `ADD COLUMN IF NOT EXISTS … DOUBLE PRECISION` with no + `DEFAULT` and `NULL`-able. PG 11+ treats this as a catalog-only DDL + (no table rewrite) — production rollout holds `ACCESS EXCLUSIVE` on + `measurement_rollups` for milliseconds, writes resume immediately. +- `0070` / `0071` are `CREATE TABLE IF NOT EXISTS` + indexes guarded by + `DO $$ BEGIN … duplicate_object` for the FKs. Idempotent on replay. + +Per-write hooks: I confirmed none of the touched routes +(`mood-entries/*`, `medications/intake/*`, `medications/[id]/intake/*`, +`mood-entries/bulk`, `medications/intake/bulk`, `ingest/medication`, +`telegram/webhook`, `import`, `integrations/moodlog/webhook`, +`settings/data`, `settings/moodlog`) wraps the entry create/update +inside an interactive `prisma.$transaction(async tx => …)`. The +admin/backups restore is the only interactive tx; its rollup +`deleteMany` correctly uses `tx`, and the long-running +`recomputeUserMoodRollups` / `recomputeUserMedicationCompliance` calls +correctly run **outside** the tx (audit-doc'd, verified in +`src/app/api/admin/backups/[id]/restore/route.ts:280-470`). The +snapshot-isolation gotcha you flagged in #2 does not apply. + +--- + +## High + +### H1 — Mood rollup DST-boundary case is documented, NOT pinned in a test + +W-MOOD report claims the UTC-bucket vs TZ-anchored `MoodEntry.date` +divergence is "pinned in parity test which uses noon-anchored +timestamps". The route parity test +(`src/app/api/mood/analytics/__tests__/route.test.ts`) uses +`moodLoggedAt: 2026-05-{08,09,10}T12:00:00.000Z` — i.e. noon UTC, far +from any DST boundary. There is NO test that exercises a Berlin DST +boundary case: + +- 2025-03-30T01:30Z (Berlin: 03:30 CEST, after the spring-forward + hour) → UTC bucket = 2025-03-30; user-tz bucket = 2025-03-30. OK. +- 2025-10-26T01:30Z (Berlin: 02:30 CET, before fall-back hour) → UTC + bucket = 2025-10-26; user-tz bucket = 2025-10-26. OK. +- 2025-10-25T23:30Z (Berlin: 00:30 CET, AFTER fall-back) → UTC bucket = + 2025-10-25; user-tz bucket = 2025-10-26. **DIVERGES.** + +For Berlin tenants the slip lands on entries logged 22:00-23:59 UTC, +which is 23:00/00:00-00:59/01:59 local depending on DST. The W-MOOD +report's noon-anchored test does not cover this. The legacy live path +emitted `MoodEntry.date` (TZ-anchored); the rollup tier emits the +UTC-midnight bucket. The route test will pass because all its +fixtures live mid-day. A real user logging "right before bed" at 23:30 +local will see the rollup bucket attribute their entry to the wrong +calendar day in the chart. + +Severity: High because it's a silent data-mislabelling, not a 5xx. +Recommend: add at least one route-parity test with +`moodLoggedAt = "2025-10-25T23:30:00.000Z"` and a user `timezone: +"Europe/Berlin"` that asserts the bucket lands on `2025-10-26` (or +explicitly accepts the documented `2025-10-25` slip). + +### H2 — Live-fallback `since` cap silently nulls `avg30LastYear` on cold mounts + +`src/app/api/analytics/route.ts:240` introduces a 90-day floor on +every per-type `fetchMeasurementSeriesChunked` call. +`summarize().avg30LastYear` reads from days 365-395 ago, so it now +returns `null` whenever the fast-path gate flips false. The W-SINCE +report flags this trade-off but the consumer impact is real: + +- `src/app/page.tsx:309` reads `summary.avg30LastYear` when + `compareBaseline === "lastYear"` and falls back to `null` → + `tileCompareDelta` returns `null` → the tile renders "no prior + data". +- `src/app/api/insights/generate/route.ts:149` and + `src/components/insights/vo2-max-chart-row.tsx:70` do the same. + +When the rollup fast-path covers the user, the rollup branch supplies +the long-window numbers via the comprehensive aggregator + summaries +slice (also 90-day capped, but `avg30LastYear` already arrives null +there — confirmed by reading +`src/lib/analytics/summaries-slice.ts:174,410,551`). So this is **not +a regression for the rollup-covered common case**; the issue is the +live-fallback path strictly worsens. + +Severity: High because the fall-through path is the one users with +brand-new measurement types or empty rollups land on, and that's +exactly when the "vs last year" tile would be a useful baseline. + +Recommend: either (a) document on the front-end that "vs last year" +silently degrades during cold mounts, or (b) leave the cap at 395+30 +days so the year-ago window remains populated on the fall-through +path. The audit's own quick-win bundle anchored on 90 days so option +(a) is a fair release-time call, but it should be Marc-Voice noted. + +### H3 — `fetchMeasurementSeriesChunked` `since` filter relies on the existing index + +The new `measuredAt: { gte: liveSince }` filter (`route.ts:697`) plus +the existing `orderBy: [{ measuredAt: "asc" }, { id: "asc" }]` is +served by the existing `(user_id, type, measured_at)` index on +`measurements`. I checked `prisma/schema.prisma:Measurement` — the +schema declares the matching `@@index([userId, type, measuredAt])` (no +diff in v1.4.39). Plan stays the same: indexed range scan, no seq +scan. **No regression.** + +The W-SUM `SUM(m."value")` addition to `runRollupAggregate`'s +`$queryRawUnsafe` (rollups.ts:391, 424) is just one more +expression in the existing `SELECT` list. The `GROUP BY m."type", +date_trunc(...)` clause and the `WHERE user_id = $1 AND type IN (...) +AND measured_at >= $2 AND measured_at < $3` predicate are unchanged. +Same index path. **No regression.** + +--- + +## Medium + +### M1 — `enqueueBootTimeRollupBackfill` UNION branch causes a seq scan on rollup table + +`src/lib/measurements/rollups.ts:762-779`: the second UNION arm + +```sql +SELECT DISTINCT r2."user_id" AS id +FROM measurement_rollups r2 +WHERE r2."granularity" = 'DAY' + AND r2."sum_value" IS NULL +``` + +has no index on `sum_value` and no anchoring predicate. On every +worker boot, this scans every `granularity = 'DAY'` row in +`measurement_rollups` and filters by `sum_value IS NULL`. For Marc's +tenant (~5-10k DAY rollup rows) this is millisecond-class. After the +column converges across all users, the predicate matches zero rows +but the seq scan still runs. + +For v1.5 multi-tenant scale this should either gain a partial index +(`CREATE INDEX … WHERE sum_value IS NULL`) or the UNION arm should be +dropped once the discovery completes. The discovery is supposed to be +idempotent and self-converging (the boot-backfill writer always +populates `sum_value`), so a follow-up release can simply drop the arm. + +Cosmetic note: the outer `SELECT DISTINCT id FROM (...) discovery` +wrapping a `UNION` is redundant — `UNION` already de-dups. Use `UNION +ALL` + outer `DISTINCT` for clarity, or just drop the outer +`DISTINCT`. Not a bug. + +### M2 — `medication_compliance_rollups.day` TEXT column lacks a tested DST-day case + +`recomputeMedicationComplianceForDay` +(`src/lib/medications/compliance-rollups.ts:177-252`) correctly +handles DST 23/25-hour days via the `startOfDayUtcInTz(nextDayKey, …)` +two-pass — the helper's `wallClockInTz` + `tzOffsetMinutes` honour +`Intl.DateTimeFormat` DST. I verified `tests/integration` and +`src/lib/medications/__tests__/compliance-rollups.test.ts` (382 +lines). + +The unit test file exercises DST-adjacent days but the assertion only +checks that the rollup row is written, not that the `[start, end)` +window correctly absorbs the 25-hour day's extra hour. For a 03:00 +local intake event on the Berlin fall-back day, both `start + +86_400_000` and the DST-safe `nextDayKey` boundary agree because +03:00 is well after the 02:00→03:00 fall-back transition. A test that +mints an event at 01:30 local on the fall-back day and asserts the +event lands in the **expected** day's bucket would close this gap. + +Severity: Medium — the writer code is correct (I traced +`windowEnd = dstSafeEnd.getTime() > start.getTime() ? dstSafeEnd : +end`), but the test does not pin the boundary, so a future refactor +of the DST helpers could silently regress without the suite catching +it. + +### M3 — Compliance scheduler mint path fires the hook per `RED`-phase row, not batched + +`src/lib/jobs/reminder-worker.ts:572-599`: for every medication whose +`existingMissed === 0` on a RED-phase scheduled-window, the worker +creates one `medicationIntakeEvent` row AND immediately calls +`recomputeMedicationComplianceForEvent`. This is the audit's question +#5 (`N+1 on scheduler mint?`). + +Across **one user with N medication schedules** firing simultaneously +the cost is N independent compliance recomputes (each is one bounded +`findMany` plus one upsert). With ~5-10 medications per power user +this is bounded and fine. + +Across **all users** the worker iterates all `med` records — but each +user's medications fire only on their own RED-phase clock. The fan-out +is naturally per-tenant. **Not** an N+1 in the +"single-batch-write-fires-N-hooks" sense. Acceptable. + +For Marc's tenant explicitly: at most 4-5 hooks per evening when +multiple medications hit their RED window inside the same worker +tick. Negligible. + +### M4 — `aggregateWmyBuckets` math is correct (validation) + +I confirmed `src/lib/measurements/rollup-read-wmy.ts:204-240` computes +`mean = sumWeighted / totalCount` where `sumWeighted = Σ(count * mean)` +and `totalCount = Σ count`. This is the correct `Σ(count·mean) / Σ +count`, NOT a naive `mean(mean)`. **Math is right.** The 18 unit +pins in `rollup-read-wmy.test.ts` cover the weighted-mean composition +(see `aggregateWmyBuckets > composes weighted mean correctly`). + +### M5 — `since` cap in `/api/analytics` does not clip the v1.5 multi-year card + +I grep'd every caller of `fetchMeasurementSeriesChunked` — only +`src/app/api/analytics/route.ts:263` uses it. The new +`computeLongWindowSummary` +(`src/lib/analytics/summaries-slice.ts:615`) routes through +`readBestGranularityRollups` (the new WMY reader), which has its own +window-day inputs. **No collision.** Finding #7 in your prompt: +clear. + +### M6 — Reader fallback discriminator is correct for both new tiers + +Mood rollup: route fallback uses `result.usedRollup` +(`src/app/api/mood/analytics/route.ts`) and the route's +`ensureUserMoodRollupsFresh` watermark check +(`src/lib/mood/rollups.ts:334-374`) reads `newestRollup.computedAt` +vs `Max(updatedAt, moodLoggedAt)`. **Robust** — not "0 rollup rows = +no data" but "rollup tier may be stale and we know which side to +trust". + +Medication rollup: `hasMedicationComplianceCoverage` +(`compliance-rollups.ts:373-389`) returns false when no rollup row +exists in the trailing window. The route then falls back to +`buildComplianceBuckets` (the legacy live aggregator) which is +correct over an empty window. The boot backfill fires on the side. +**Robust** for the "zero events" and "zero rollup but events exist" +cases. The "rollup exists but is stale" case relies on the per-write +hooks keeping it fresh — same posture as the measurement rollups. + +--- + +## Low + +### L1 — Cosmetic SQL clean-up in the boot-backfill UNION + +See M1 — `SELECT DISTINCT id FROM (... UNION ...)` wraps a `UNION` +which already de-dups. Minor. + +### L2 — `recomputeMoodBucketsForEntry` only flushes WEEK/MONTH/YEAR on enqueue + +Same posture as `measurement-rollups`: the DAY pass is synchronous, +the rest is fire-and-forget via pg-boss. Worth noting that the route +test parity (which mocks pg-boss) does not assert the WEEK/MONTH/YEAR +buckets get enqueued — if a future refactor drops the `Promise.all` +fan-out, no test would catch it. Reachable via a `boss.send` spy +assertion, not currently included. + +### L3 — Compliance rollup `day` TEXT range scans for trailing window + +`hasMedicationComplianceCoverage` and `readMedicationCompliance` use +`day: { gte: oldestKey, lte: newestKey }` with `day` as TEXT. The +primary key is `(user_id, medication_id, day)` and the descending +index is on `(user_id, day DESC)`. The TEXT comparison is correct for +ISO `YYYY-MM-DD` strings (lexicographic == chronological), so the +range scan is index-served. **No regression.** Worth noting for +future maintenance because TEXT day-keys are unusual in this codebase. + +--- + +## Tests delta + +W-MOOD: +21 unit/route tests. W-MED: +18. W-SUM: +8 across 3 +test files. W-WMY: +23. W-SINCE: +3. Net ~ +73 v1.4.38.8 → develop. + +I sampled the writer tests for parity contracts and they correctly pin +the new `sum_value` flow-through, the rollup partition deletion on +last-row removal, and the weighted-mean composition. + +--- + +## Brief-back (≤ 200 words) + +**No critical issues.** Migrations are non-blocking (column add is +catalog-only on PG 11+). Per-write hooks correctly call against the +right Prisma client; no `tx` snapshot-isolation traps. `aggregateWmyBuckets` +correctly weights mean by count. `since` cap does not clip the v1.5 +multi-year card (only the legacy analytics route's live-fallback). + +**Two high findings worth Marc's call before tagging:** + +1. **H1** — the mood rollup DST-boundary case is documented but not + pinned in a test. The "noon-anchored" parity test misses the + 23:30-01:00 local-tz boundary that diverges UTC vs user-tz day-keys. + Recommend adding one boundary-case route-parity test before + release. + +2. **H2** — the `/api/analytics` 90-day `since` cap silently nulls + `summary.avg30LastYear` on the live-fallback path, breaking the + dashboard / VO2 chart / insights generator's "vs last year" + comparison tile when the fast-path gate flips false. The rollup + common case is unaffected. Recommend either Marc-Voice user-facing + note or widening the cap to ~425 days to preserve the year-ago + window. + +**One medium recommendation for v1.4.40:** drop the +`measurement_rollups.sum_value IS NULL` UNION arm from the boot +backfill once it has converged across all users (M1). Currently runs a +seq scan on every worker boot. diff --git a/.planning/round-v1439-QA-ux-findings.md b/.planning/round-v1439-QA-ux-findings.md new file mode 100644 index 000000000..34e736f7f --- /dev/null +++ b/.planning/round-v1439-QA-ux-findings.md @@ -0,0 +1,229 @@ +# v1.4.39 — UX / Response-Shape Review (read-only) + +Reviewer scope: byte-shape parity on the four read-path swaps + the +new `computeLongWindowSummary` helper. Diff base +`v1.4.38.8..develop`; agent reports +`phase-W-MOOD-v1439-report.md`, `phase-W-MED-v1439-report.md`, +`phase-W-SUM-v1439-report.md`, `phase-W-WMY-v1439-report.md` read. + +The four target route handlers + test files inspected: +- `src/app/api/mood/analytics/route.ts` (+ `__tests__/route.test.ts`) +- `src/app/api/medications/intake/route.ts` (+ `__tests__/route.test.ts`) +- `src/app/api/dashboard/summary/route.ts` (+ `__tests__/route.test.ts`) +- `src/app/api/measurements/route.ts` (+ `__tests__/range-aggregation-route.test.ts`) + +Helpers checked: `readMoodDayRollups`, `readMedicationCompliance`, +`hasMedicationComplianceCoverage`, `readBestGranularityRollups`, +`computeLongWindowSummary`, `resolveBucketSum`. + +--- + +## SEV: HIGH + +### H1 — Mood `summary.count / latest / min / max / mean / avg7 / avg30` semantics shift on multi-entry days (live → rollup divergence) + +File: `src/app/api/mood/analytics/route.ts:97-101` + +The legacy live path called `summarize()` over **per-entry** +`DataPoint`s (`moodLoggedAt`, `score`). The new rollup fast-path +calls `summarize()` over **per-day mean** `DataPoint`s +(`bucketStart`, `r.mean`). For a user with two entries on the same +day (score 7 + score 8), the legacy response emitted: + +| field | legacy | rollup | +|---------|-------------------|---------------------| +| count | total entries | total days | +| latest | last entry's score| last day's mean | +| min/max | per-entry min/max | per-day-mean min/max| +| mean | unweighted avg of all entries | unweighted avg of daily means (Simpson-shift on uneven day counts) | +| slope* | OLS over per-entry x = elapsed ms | OLS over per-day x | + +The route comment at lines 91-96 acknowledges this and asserts "power +user's mood is typically 1/day" so the divergence is invisible. That +assumption is reasonable for Marc's tenant but **is** a contract +shift. Consumers on `summary.count` (`src/app/page.tsx:1195` — +dashboard sort gate, harmless) and `summary.latest / avg7 / avg30 / +slope30` (`src/app/page.tsx:878-883` — `TrendCard` tile) read these +fields directly. + +Existing parity test `emits byte-identical entries + summary shape +between rollup tier and live fallback` only covers one-entry-per-day +data so the divergence is uncovered. Add a multi-entry-per-day pin +that either (a) accepts the new semantics with a documented contract +or (b) pre-aggregates the live-fallback path through +`aggregateDailyAverages` first so the rollup and live shapes match +on multi-entry days too. + +Recommendation: option (b) — make the live-fallback walk fold daily +means before `summarize()` so the two branches stay byte-identical. +Cheap, single map call, removes the silent-shift risk entirely. + +--- + +## SEV: MEDIUM + +### M1 — Compliance reader may emit `days + 1` buckets on DST fall-back edge + +File: `src/lib/medications/compliance-rollups.ts:330-332` + +```ts +if (!dedup.has(todayKey)) { + orderedKeys.push(todayKey); +} +``` + +Defensive "always include today" insert that runs **after** the +`for (let i = days - 1; i >= 0; i--)` window builds `orderedKeys`. +On a normal day the loop already includes `todayKey`, so the guard +is a no-op. On a DST fall-back when the loop dedups two probes onto +the same day-key, this branch pushes a fresh entry **outside the +trailing window** if the dedup ever drops `todayKey` itself — +returning `days + 1` buckets where the legacy route always returned +exactly `days`. + +Coverage gap: `compliance buckets for the last N days` parity test +pins `body.data.length === 7` on a stable day, not on DST. Live +fallback (`buildComplianceBuckets`) always emits exactly `days`. Add +a DST-fall-back pin and either drop the defensive insert or cap the +final array at `.slice(0, days)`. + +### M2 — Mood `entries[].score` rounding can diverge by ≤0.005 on multi-entry days + +File: `src/app/api/mood/analytics/route.ts:86` + +Legacy: `Math.round((stats.sum / stats.count) * 100) / 100` — JS +arithmetic over integer scores. +Rollup: `Math.round(r.mean * 100) / 100` where `r.mean` is Postgres +`AVG(score)::double precision` — IEEE-754 double through a +different precision chain. + +For mood scores (1-5 integers) the two paths produce identical +two-decimal rounding in essentially every realistic case, but the +intermediate float math is not provably identical. Worth a property +test pinning a fixture where JS-AVG and PG-AVG could disagree at the +fifth decimal (e.g. seven entries: 3, 3, 3, 4, 5, 5, 5 → 4.0 +vs 4.0). Currently uncovered; risk is cosmetic (Recharts axis tick +shifts on the third decimal). + +### M3 — `entries[].samples` semantics consistent BUT route-meta `entryCount` semantics shift + +File: `src/app/api/mood/analytics/route.ts:102, 139` + +Rollup fast-path: `entryCount = rollups.reduce((s, r) => s + r.count, 0)` — total raw entries across all days. +Live fallback: `entryCount = moodEntries.length` — total raw entries. + +These match. Good. **But** `entryCount` is only emitted into +`annotate({ meta })` — not into `apiSuccess(...)` — so it never +reaches the client. No client-facing shape impact; flagging for +audit completeness only. + +### M4 — `/api/analytics` live-fallback 90-day cap shifts `summary.avg30LastYear` to null for power users + +File: `src/app/api/analytics/route.ts:240-269` + +Out of the brief's strict 4-route scope but adjacent: commit +`823føbc8 perf(analytics): cap live-fallback findMany at trailing +90 days` caps every per-type chunked read at the trailing 90 days. +Any user who lands on the live-fallback path (i.e. the rollup +coverage probe missed) loses `summary.avg30LastYear` (window +365-395 d ago) and `summary.avg30LastMonth` (30-60 d ago is +preserved by the 90-day window). + +The route comment at `route.ts:230-239` documents this as an +intentional trade-off matching the slim slice's existing 90-day +contract. **Shape parity is preserved (the field is still emitted, +just `null`)** so the iOS / web consumer code handles it +gracefully. Flagged because the marathon brief asked for +"pagination / limit preservation" — there is no pre-existing limit +on this path, but the new 90-day cap is a behavioural shift the +dashboard tile's comparison-overlay will surface. + +--- + +## SEV: LOW + +### L1 — `computeLongWindowSummary` not yet exposed via any route (confirms brief) + +File: `src/lib/analytics/summaries-slice.ts:615` + +`computeLongWindowSummary` is exported but **not consumed from any +route handler or server-component**. `grep -rn` on `src/app` +confirms zero callers. Reserved for v1.5 multi-year trend card and +Coach drawer per `phase-W-WMY-v1439-report.md`. No exposure risk. + +### L2 — Locale-sensitive formatting absent on the swap surfaces + +No new code in the four target routes does locale-aware date +formatting. Mood rollup labels via `utcDayLabel(d)` which uses pure +`getUTC*` calls (no `toLocaleDateString`). Compliance rollup keys +use `userDayKey(date, tz)` from `tz/format` — same helper the legacy +route used. No `de-DE` vs `en-US` bucket shift risk surfaced. + +### L3 — Backend-only paths emit no user-facing strings + +No `t(...)` / translator calls introduced into the four routes' new +code paths. Coach evidence chips, i18n labels, all client-side. No +i18n exposure regression. + +### L4 — Sort-order parity holds across all swap surfaces + +- `readMoodDayRollups`: `orderBy: { bucketStart: "asc" }` — matches + legacy `aggregateDailyAverages` final `sort([a, b]) => + a.localeCompare(b)`. +- `readMedicationCompliance`: builds keys oldest-first then sorts + via `orderedKeys` array order. Legacy `buildComplianceBuckets` + ended in `.sort((a, b) => a.date.localeCompare(b.date))`. Both + produce ISO `YYYY-MM-DD` ascending. Match. +- Dashboard `sparkBuckets`: `bucket_start ASC` in raw SQL. + Unchanged. +- Measurements `source=rollup` branch: `orderBy: { bucketStart: + "asc" }`. Unchanged. + +### L5 — Empty-state shape parity holds across all swap surfaces + +- Mood: both branches return `{ entries: [], summary: + summarize([]) }` where `summarize([])` is the canonical null + envelope (`count: 0, latest: null, ...`). Pinned in test + `returns an empty envelope for a brand-new user`. +- Compliance: both branches produce zero-filled `days`-length + array. Pinned for legacy path; new path falls through to it on + coverage-miss. +- Dashboard sparkline: `sparkOf(type)` returns `[]` on miss; tile + consumer renders empty chart. +- Measurements rollup: `rollupRows.length > 0` gate falls through + to legacy `date_trunc` path on miss; shape preserved. + +### L6 — Pagination / `limit` preservation + +- Mood: legacy had no limit (unbounded); new path applies a 5-year + `since` filter (~1 800 row ceiling). Documented in route comment. + Strictly tighter, not a regression — but the legacy + "I have 6-year-old mood entries" power user would lose the + ≥5-year-old tail. Realistically Marc started logging mood in + 2023 so this is theoretical. +- Compliance: legacy passed `days` straight through; new path + preserves `days` and clamps at 365 via Zod. Same surface. +- Dashboard sparkline: `SPARK_DAYS = 7` unchanged. +- Measurements: `take: cap = Math.min(limit, BUCKET_CAP.daily)` + unchanged. + +--- + +## Summary + +| Sev | Count | Themes | +|-----|-------|--------| +| H | 1 | Mood summary shape shifts on multi-entry days (uncovered) | +| M | 4 | Compliance DST edge, mood rounding, route-meta, analytics 90 d cap | +| L | 6 | Helper not exposed yet, locale, i18n, sort, empty-state, limits | + +The single H finding is **the** UX risk worth blocking on: power-user +multi-entry-mood-day tiles will silently shift `latest / mean / avg7 +/ avg30 / slope30` when the rollup tier converges. The fix is +mechanical (pre-aggregate the live-fallback path through +`aggregateDailyAverages` before `summarize()`) and the parity test +just needs one multi-entry fixture to lock it in. + +All four routes preserve byte-shape for empty state, sort order, +limits, locale-independence, and i18n absence. The new +`computeLongWindowSummary` helper is correctly NOT exposed. diff --git a/.planning/round-v1440-backlog.md b/.planning/round-v1440-backlog.md new file mode 100644 index 000000000..93e1cd250 --- /dev/null +++ b/.planning/round-v1440-backlog.md @@ -0,0 +1,198 @@ +# v1.4.40 backlog + +Seeded during the v1.4.39 QA reconcile. Items the six QA reviewers +surfaced that the reconcile deliberately deferred — either too costly +for the v1.4.39 release window, or naturally paired with a v1.5 +architectural step. Source-doc references in parentheses so the next +session can re-read the original rationale. + +--- + +## High — strategic follow-up + +### F-SEC-M-02 — Postgres RLS for the two new rollup tables +(`.planning/round-v1439-QA-security-findings.md` MEDIUM-F-SEC-M-02) +- `prisma/migrations/0070_v1439_mood_rollups`, + `prisma/migrations/0071_v1439_medication_compliance_rollups` — + `mood_entry_rollups` and `medication_compliance_rollups` carry no + per-user RLS or query enforcement; every read relies on + application-layer `userId` filters. +- Inspected: 100% of new Prisma reads include `where: { userId }` and + 100% of new upsert/deleteMany calls scope to userId. No leak found. +- Background: this is consistent with the pre-existing + `measurement_rollups` posture, but it remains the only + tenant-isolation layer for the two new tables. Worth defense-in-depth + RLS before v1.5 multi-org work lands. + +### F-M-01 — `computeLongWindowSummary` consumer wiring (v1.5) +(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-01) +- `src/lib/analytics/summaries-slice.ts:615` — exported + tested but no + production caller. W-WMY shipped the helper for a v1.5 multi-year + trend card / Coach drawer that doesn't exist yet. +- Action: wire into the v1.5 "year-in-mood" tile or Coach long-window + prompt; until then this is dead-but-tested code in the production + bundle. + +--- + +## Medium — correctness + perf polish + +### F-M-02 — Drop unused `tx` parameter from rollup writers +(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-02) +- `src/lib/mood/rollups.ts:173-178`, + `src/lib/medications/compliance-rollups.ts:177-183` — both writers + accept a `tx` parameter but no call site passes one. The + atomic-with-parent story in the wave reports is therefore + unrealised; every rollup write happens AFTER the parent commits. +- Action: drop the `tx` parameter or wire it through the existing + `prisma.$transaction` blocks (e.g. the Telegram skip webhook). The + v1.4.39 atomic-upsert hotfix (QA F-H-02) replaced the JS aggregate + with `$executeRaw`, so the cross-Prisma-client typing matters less + now, but the parameter is still dead. + +### F-M-03 — Add partial index `sum_value IS NULL` post-convergence +(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-03, +`.planning/round-v1439-QA-specialist-findings.md` MEDIUM-M1) +- `src/lib/measurements/rollups.ts:762-780` — boot-backfill discovery + UNION arm `WHERE r2."granularity" = 'DAY' AND r2."sum_value" IS NULL` + does a seq scan over `measurement_rollups`. Negligible at single- + tenant scale (~few thousand rows). For v1.5 multi-tenant scale this + should either gain a partial index + `CREATE INDEX CONCURRENTLY ON measurement_rollups(user_id) + WHERE granularity = 'DAY' AND sum_value IS NULL` once the legacy + NULL backfill finishes the first sweep, or the UNION arm should be + dropped (see Specialist-M01 below). + +### F-M-05 — Consolidate `RollupBucketRow` shape +(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-05) +- `src/lib/measurements/rollup-read.ts` exports `DailyMeanRow`, + `src/lib/measurements/rollup-read-wmy.ts` exports `RollupBucketRow`. + The latter is a strict superset (adds `sd/slope/r2/sumValue`). Two + coexisting bucket-row types invite drift. +- Action: consolidate into one shape in `rollup-read.ts` and re-export + from `rollup-read-wmy.ts`. Cheap. + +### F-M-06 — Per-agent worktree for next marathon (process note) +(`.planning/round-v1439-QA-senior-findings.md` MEDIUM-F-M-06, +`.planning/round-v1439-QA-product-lead-findings.md` Risk register) +- W-SUM's tests landed inside W-WMY's commit `8763b3aa`; W-MOOD's + `d15850d5` overwrote W-MED's reminder-worker.ts edits. Third + recurring occurrence of cross-agent commit-attribution drift. +- Action: per-agent `git worktree` in the next marathon — add + worktree-per-wave to the `release-marathon` skill's standard flow. + +### UX-M1 — Compliance reader DST `days+1` slice +(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M1) +- `src/lib/medications/compliance-rollups.ts:330-332` — defensive + "always include today" insert runs after the trailing-window loop. + On a normal day it's a no-op; on a DST fall-back when the loop dedups + two probes onto the same day-key, this branch may push a fresh entry + outside the trailing window, returning `days + 1` buckets. +- Action: add a DST-fall-back pin and either drop the defensive insert + or cap the final array at `.slice(0, days)`. + +### UX-M2 — Mood entry-score rounding pin +(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M2) +- `src/app/api/mood/analytics/route.ts:86` — rollup path uses + `Math.round(r.mean * 100) / 100` over PG `AVG`; legacy live uses + `Math.round((stats.sum / stats.count) * 100) / 100`. JS-AVG vs PG-AVG + intermediate float math is not provably identical. +- Action: property test pinning a fixture where the paths could + disagree at the fifth decimal. Cosmetic risk only (Recharts axis + ticks). + +### UX-M3 — `entryCount` shape decision (info-only) +(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M3) +- `src/app/api/mood/analytics/route.ts:102, 139` — `entryCount` + semantics consistent across both branches (total raw entries) but + the field only lands in `annotate({ meta })`, not `apiSuccess()`. + No client-facing shape impact; flagging for audit completeness only. + +### Specialist-M01 — Drop `sum_value IS NULL` UNION arm after convergence +(`.planning/round-v1439-QA-specialist-findings.md` MEDIUM-M1) +- `src/lib/measurements/rollups.ts:762-779` — once Marc's tenant has + converged, the legacy-NULL branch becomes a permanent no-op walk over + the full rollup table. Schedule for removal in v1.4.40 + add a TODO + with the cut-off date. Keeps the discovery query single-table after + convergence. + +### Simplifier dead-exports + duplicate tz helpers +(`.planning/round-v1439-QA-simplifier-findings.md` Suggested removals + +Suggested dedups) +- `src/lib/measurements/rollup-read-cumulative.ts:54,83,121,164` — + `isCumulativeType`, `readCumulativeDaySums`, `readCumulativeDaySumsBatch`, + `resolveBucketSum` exported + tested but no production caller. ~360 + lines if file + tests collapse. Once a consumer exists, this becomes + the single source of truth. +- `src/lib/measurements/rollup-read-wmy.ts:125,137,149` — + `readWeekRollups` / `readMonthRollups` / `readYearRollups` only used + by their own tests; downgrade to non-exported or inline. +- `src/lib/medications/compliance-rollups.ts:453` — + `ensureUserMedicationComplianceFresh` only referenced in its own + JSDoc; the v1.4.39 QA F-SEC-M-01 split shipped + `enqueueUserMedicationComplianceBackfill` so the original `ensure- + fresh` symmetry helper is dead. Drop or wire. +- Duplicate tz helpers (`wallClockInTz`, `tzOffsetMinutes`, + `startOfUtcDay`, `bucketSpan`, `startOfDayUtcInTz`) — three to four + copies across `src/lib/measurements/rollups.ts`, `src/lib/mood/ + rollups.ts`, `src/lib/medications/compliance-rollups.ts`, + `src/lib/medications/scheduling/cadence.ts`. Consolidate into + `src/lib/tz/format.ts` (Marc-Voice: "one tz helper, one bug fix + radius"). + +### UX-M4 — Front-end `vs last year` degradation hint +(`.planning/round-v1439-QA-ux-findings.md` MEDIUM-M4) +- Now **obsolete** after the v1.4.39 QA Specialist-H2 widen + (`/api/analytics` live cap 90d → 425d). The "vs last year" tile no + longer silently nulls on cold mounts. Flagged here so a future + reviewer doesn't reopen. + +--- + +## Low — observability polish + +### F-L-01 — Compliance double-window-derivation cleanup +(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-01) +- `src/lib/medications/compliance-rollups.ts:186-199` — + `recomputeMedicationComplianceForDay` computes the day window twice + (`start + 86_400_000` and `startOfDayUtcInTz(nextDayKey)`) then + picks the larger. Simpler invariant: "always use `startOfDayUtcInTz` + for both bounds". Cosmetic. + +### F-L-03 — Mood-rollup chunked upsert serialisation +(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-03) +- `src/lib/mood/rollups.ts:526-560` — `persistMoodRollupRows` chunks + at 500 but issues a sequential `for…of` upsert loop inside each + chunk. Measurement-rollups equivalent uses + `prisma.$transaction([…])` for parallel upsert. Mood DAY hot-path + writes 1 row so it doesn't matter; a 5-year boot-fold pays N + round-trips instead of N/500 transactions. + +### F-L-04 — Mood boot-backfill discovery query clarification +(`.planning/round-v1439-QA-senior-findings.md` LOW-F-L-04) +- `src/lib/mood/rollups.ts:417-427` — discovery query doesn't restrict + `granularity='DAY'` on the LEFT JOIN. Inconsequential because the + read path only consults DAY anyway. Comment clarification would help. + +### F-SEC-L-01 — Compliance recompute silent-error observability +(`.planning/round-v1439-QA-security-findings.md` LOW-F-SEC-L-01) +- `src/lib/medications/compliance-rollups.ts:277-289` — + `recomputeMedicationComplianceForEvent` swallows errors with an + `annotate({ medication_compliance_rollup_failed: true })` flag. + Self-corrects on the next mutation; no PII logged. Worth a follow-up + metric / alert on the annotate flag. + +--- + +## Strategic note (carry into v1.5) + +- iOS Health-app integration remains v1.5 P1; the v1.4.39 backlog + items above are all internal correctness/perf polish that doesn't + gate iOS work. +- Cross-tz proper fix (per-user-tz rollup bucket minting) remains + v1.5 — the v1.4.38 cheap path (runtime guard with live fallback for + non-near-UTC) is in place; v1.4.39 mood + compliance rollups inherit + the UTC-anchor trade-off pinned by Specialist-H1's DST test. +- Per-source rollup (audit P5) + slope-window SQL move (audit P8) + remain v1.5 architectural backlog — unchanged from the v1.4.38 + closure.