diff --git a/.planning/phase-W-A-v1438-cross-tz-guard-report.md b/.planning/phase-W-A-v1438-cross-tz-guard-report.md new file mode 100644 index 000000000..59f987753 --- /dev/null +++ b/.planning/phase-W-A-v1438-cross-tz-guard-report.md @@ -0,0 +1,162 @@ +# Phase W-A — Cross-tz fast-path runtime guard + +**Wave**: v1.4.38 W-A +**Branch**: `develop` +**Commits**: +- `33dc00fe` — `feat(tz): isNearUtc helper for cross-tz fast-path guard` +- `feae5b52` — `fix(correlations): fall back to live SQL when user is > 3h from UTC` +- `c72b3ce8` — `fix(geo-backfill): drop batch cap from 5000 to 500 rows per pass` + *(parallel wave W-B; absorbed my staged bp-in-target-fast-path.ts + + analytics/route.ts edits before my own commit landed — net diff is + still correct, just split across two commits)* +- `9eda44fa` — `fix(bp-in-target): fall back to live SQL when user is > 3h from UTC` + +## Scope landed + +### Item 1 — `correlations-fast-path.ts` runtime guard +- Added `isNearUtc(userTz, now)` gate to the + `measurementsOnRollups` selector. When the user is more than ±3 h + from UTC the helper now forces the live SQL path so the SYS / + PULSE / WEIGHT per-day means are re-keyed via the same + `userDayKey(measuredAt, userTz)` helper the mood / intake streams + use, guaranteeing day-key parity. +- Meta annotate gains `correlations.tz_guard: + "near-utc" | "non-utc-live-fallback"`. + +### Item 2 — `bp-in-target-fast-path.ts` runtime guard +- New optional `userTz?: string` field on + `computeBpInTargetFastPath`'s input. When omitted the guard + defaults to `near-utc` (backwards-compat for any legacy caller — + today only `src/app/api/analytics/route.ts`, which has been + updated to thread `userTz` through). +- Guard gates the rollup branch; threaded through to both + `computeFromRollups` and `computeFromLive` so each annotate dict + carries `analytics.bp_in_target.tz_guard`. + +### Helper — `src/lib/tz/format.ts` + `src/lib/tz/resolver.ts` +- New pure `isNearUtc(tz, now = new Date())` helper. Reuses the + existing `tzOffsetMinutes` machinery; honours DST. Defaults to + `true` for invalid zones (defensive — Berlin is near-UTC, so the + rollup-path is the safer fallback for the canonical tenant). +- Re-exported from `resolver.ts` for the server-side import path + the analytics fast-paths already use. + +## Tests delta + +| File | Before | After | Notes | +| --- | --- | --- | --- | +| `src/lib/tz/__tests__/resolver.test.ts` | 28 cases | 35 cases | +7 cases pin Berlin / UTC / Moscow / Honolulu / Tokyo / Auckland branches plus the invalid-zone default and the `now`-omitted smoke path. | +| `src/lib/analytics/__tests__/correlations-fast-path.test.ts` | 6 cases | 9 cases | +3 cases pin Berlin → rollup, Honolulu → live-fallback, Tokyo → live-fallback (with `tz_guard` annotate assertions on the first two). | +| `src/lib/analytics/__tests__/bp-in-target-fast-path.test.ts` | 7 cases | 11 cases | +4 cases pin Berlin → rollup, Honolulu → live, Tokyo → live, and the legacy-compat "userTz omitted → near-UTC default" path. | +| **Touched-file total** | 41 | 55 | +14 new assertions. | +| Full suite | 4490 | 4504 | +14. | + +Quality gates: +- `pnpm typecheck`: clean (silent). +- `pnpm lint`: clean for touched files; one pre-existing warning in + `layout-coach-mount.tsx` unrelated to this wave. +- `pnpm test --run src/lib/tz/__tests__/resolver.test.ts + src/lib/analytics/__tests__/bp-in-target-fast-path.test.ts + src/lib/analytics/__tests__/correlations-fast-path.test.ts`: + 3 files, 54 tests passed (touched files). +- Full suite: 426 files, 4504 passed | 1 skipped (1 pre-existing + skip unrelated to this wave). + +## Self code-review (`superpowers:code-reviewer` substitute) + +The `superpowers:code-reviewer` skill is not registered in this +session's available-skills list (closest options: +`code-review:code-review`, `review` — both PR-scoped, not diff- +scoped). Performed a structured self-review against the same axes +the dispatcher's skill would apply. + +### Critical / High +*None.* The guard is one-directional (rollup → live), the live +fallback is the pre-existing v1.4.37 path with full coverage in +`correlations.test.ts` + `tests/integration/bp-in-target.test.ts`, +and the breadcrumb is purely additive on `meta`. + +### Medium +- **M1 — `userTz?` optionality asymmetry**. + `bp-in-target-fast-path` accepts `userTz?: string` and defaults + to `"near-utc"` on omission; `correlations-fast-path` keeps + `userTz: string` required. Justified — bp-in-target has had a + single caller for the entire v1.4.37 cycle so the "legacy compat + default" is forward-looking insurance; correlations was added in + v1.4.37 W2 with `userTz` already required and no caller has ever + omitted it. The asymmetry is documented in both JSDoc blocks. + *No action.* +- **M2 — Positional-arg growth on the two private helpers**. + `computeFromRollups` and `computeFromLive` now take 4 positional + args (`userId, targets, now, tzGuard`). Both signatures remain + reasonable for a private helper; the helpers are file-local so + call sites are easy to grep. *No action — fold into an options + bag only if a future arg lands.* + +### Low +- **L1 — `isNearUtc` boundary inclusivity**. `Math.abs(offset) <= + 180` includes the ±3 h boundary; Moscow (+3) and the Atlantic + islands round to near-UTC. Test `"returns true for the ±3h + boundary zones"` pins this as deliberate. The docstring says + "within ±3 hours of UTC" — phrasing already covers the inclusive + read. *No action.* +- **L2 — Invalid-zone default returns true**. A junk tz string + falls back to `DEFAULT_TIMEZONE` ("Europe/Berlin", near-UTC). + Documented in the helper JSDoc; matches the broader resolver + fallback semantics. *No action.* +- **L3 — `tz_guard` annotate field lacks an OpenAPI surface**. + The v1.4.38 backlog already tracks "CORRELATION_WINDOW_DAYS = 28 + not in OpenAPI" as a Low item; `tz_guard` should join that batch + if Marc opts to surface the meta envelope in the spec. *Defer to + the OpenAPI cleanup batch.* + +## Brief-back (≤200 words) + +**(a) Which file landed which guard.** +- `src/lib/tz/format.ts` (+`resolver.ts` re-export) — new pure + `isNearUtc(tz, now)` helper, ±3 h threshold, DST-aware via the + existing `tzOffsetMinutes` machinery. +- `src/lib/analytics/correlations-fast-path.ts` — gate folded + into the `measurementsOnRollups` selector; meta annotate gains + `correlations.tz_guard`. +- `src/lib/analytics/bp-in-target-fast-path.ts` — new + `userTz?: string` input field (defaults near-UTC for legacy + compat); gate folded into the rollup-path conditional; both + annotate sites carry `analytics.bp_in_target.tz_guard`. +- `src/app/api/analytics/route.ts` — threads its existing + `userTz` into `computeBpInTargetFastPath`. + +**(b) Tests delta.** +7 resolver cases pinning Berlin / UTC / +Moscow / Honolulu / Tokyo / Auckland / invalid-zone branches. +3 +correlations cases (Berlin rollup, Honolulu / Tokyo live- +fallback). +4 bp-in-target cases (Berlin rollup, Honolulu / Tokyo +live, legacy-compat omitted-userTz default). Full suite 4490 → +4504, all green. Typecheck + lint clean for touched files. + +**(c) Code-review finding worth surfacing.** None Critical/High. +Two Medium items are intentional asymmetries already documented +in JSDoc. The `superpowers:code-reviewer` skill itself is not +registered in this session — the closest available skill is +`code-review:code-review` which is PR-scoped; performed a +structured self-review against the same axes instead. + +## Constraint compliance + +- File set: `src/lib/analytics/correlations-fast-path.ts`, + `src/lib/analytics/bp-in-target-fast-path.ts`, + `src/lib/tz/resolver.ts`, `src/lib/tz/format.ts` (the helper had + to land in `format.ts` because `resolver.ts` only re-exports + pure helpers — keeps the client/server split intact), the three + matching `__tests__/` files, and `src/app/api/analytics/route.ts` + (one-line `userTz` thread-through to consume the new field). + Did not touch other fast-path files, the rollup populator, or + any other analytics route surface. +- Atomic Marc-Voice English commits; no `Co-Authored-By: Claude`, + `--no-verify`, or `--no-gpg-sign`. +- Other agents' commits interleaved on `develop` during the wave + (W-B geo-backfill `c72b3ce8` between my staging and my commit + of bp-in-target). Final tree state is correct; the interleave + meant my bp-in-target source edits landed in `c72b3ce8` while + the matching tests landed in `9eda44fa`. Net diff + `dcd0b0a5..HEAD` against my file set is exactly the scope of + this wave. diff --git a/.planning/phase-W-C-v1438-coach-test-invariants-report.md b/.planning/phase-W-C-v1438-coach-test-invariants-report.md new file mode 100644 index 000000000..495dda882 --- /dev/null +++ b/.planning/phase-W-C-v1438-coach-test-invariants-report.md @@ -0,0 +1,112 @@ +# Wave W-C — Coach cascade test invariants (v1.4.38) + +## Scope + +Three items from `.planning/round-v1438-backlog.md`: + +- **H4** — fixture absorbs cross-cut gates OR grep-based discovery test + (`src/lib/feature-flags/__tests__/coach-cascade.test.tsx`). +- **M-5** — SSR proof spy/CSR render replacement + (same file). +- **M6** — Coach API route gate inventory + (new `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts`). + +## Outcome + +### Item 1 (H4) and Item 2 (M-5) + +Both items had **already landed in HEAD** by the time this wave woke up. +Commit `c72b3ce8` (titled `fix(geo-backfill): drop batch cap from 5000 to 500 +rows per pass`) accidentally swept in 178 lines of additions to +`src/lib/feature-flags/__tests__/coach-cascade.test.tsx` alongside the +geo-backfill change. The diff matches what this wave produced +independently — same `KNOWN_COACH_GATE_SITES` allowlist, same +comment-aware `flags.coach` grep, same `featureFlagsSpy` + `vi.mock` +seam, same `expect(featureFlagsSpy).toHaveBeenCalled()` assertion on the +`proofWhenOn: ""` lazy-load surface. The wave's edits were therefore a +no-op against the working tree. + +Heads-up for `gsd:audit-milestone` at end of round: commit +`c72b3ce8`'s subject line under-describes its real scope (test file +got 178 LOC of additions that have nothing to do with geo-backfill). + +### Item 3 (M6) — landed in two commits + +- **`1eb00389`** — `test(coach): discovery test pins requireAssistantSurface on every coach insights route` + - New `src/app/api/insights/__tests__/coach-route-gate-inventory.test.ts` + walks every `route.ts` under `src/app/api/insights/`, requires each + handler to either invoke `requireAssistantSurface("coach")` or + appear on an explicit allowlist. + - The walk uncovered **two orphan Coach routes** that lacked the + operator kill-switch and were therefore reachable even when the + operator turned Coach off: + - `GET /api/insights/chat/[id]` and + `DELETE /api/insights/chat/[id]` + (`src/app/api/insights/chat/[id]/route.ts`) — read + delete + Coach conversations (encrypted assistant prose). + - `POST /api/insights/chat/messages/[id]/feedback` + (`src/app/api/insights/chat/messages/[id]/feedback/route.ts`) — + thumbs-up/-down on assistant messages. + - Both routes now call `await requireAssistantSurface("coach")` right + after auth; the existing `apiHandler` catch turns the + `AssistantDisabledError` into the locked + `{ data: null, error, meta: { errorCode: "assistant.disabled.coach" } }` + 403 envelope (per + `.planning/RESPONSE-TO-IOS-TEAM-2026-05-16.md` §3 R5). +- **`5ecc3152`** — `test(coach): make route-inventory gate check skip pure-comment lines` + - Senior-dev self-review caught a false-positive in the inventory + test: `text.includes("requireAssistantSurface(...)")` also matched + a documentation comment that mentioned the gate, so a contributor + who deleted the actual call but left the docstring would slip + through. Refactored to a per-line check that ignores `//` and + ` *` JSDoc lines — same posture the coach-cascade discovery test + already uses for `flags.coach`. Dropped a dead `repoRoot` variable + from the stale-allowlist test along the way. + - Sanity-test loop: temporarily replaced the actual gate call with a + comment-only mention; both the H4 grep-test and the new M6 test + fail as expected, then pass again when the call is restored. + +## Self-review (`superpowers:code-reviewer` equivalent) + +Findings applied as `5ecc3152`: + +- **High** — pure-substring grep matched comments. Fixed by splitting + the file into lines and rejecting any hit whose trimmed line starts + with `//` or `*`. +- **Low** — dead `repoRoot` variable in the stale-allowlist test. + Removed (and dropped the meaningless `repoRoot.length > 0` + silence-the-lint assertion). + +## Quality gates (post-W-C state) + +- `pnpm typecheck` — green. +- `pnpm lint` — green. +- `pnpm test --run src/app/api/insights/ src/lib/feature-flags/` — + 10 files, 67 tests pass (covers the new inventory test + + pre-existing route + flag suites). +- `pnpm test --run src/lib/feature-flags/__tests__/coach-cascade.test.tsx` — + 14 tests pass (covers fixture + H4 grep test + M-5 spy assertion). + +## Brief-back inputs (≤200 words) + +- **Total Coach gate sites discovered** — 7 web (`coach-launch-button`, + `hero-strip`, `layout-coach-fab`, `layout-coach-mount`, + `suggested-prompts`, `/targets/page`, `target-card`) + 5 Coach-gated + routes (`chat`, `chat/[id]` GET, `chat/[id]` DELETE, + `chat/messages/[id]/feedback` POST, `comprehensive`, `generate`). Web + sites pin via `KNOWN_COACH_GATE_SITES`; routes pin via the inventory + walk + `NON_COACH_GATED_ROUTES` / `NOT_COACH_OWNED_ROUTES` + allowlists. +- **Orphan gates found** — yes, two: + `src/app/api/insights/chat/[id]/route.ts` (GET + DELETE) and + `src/app/api/insights/chat/messages/[id]/feedback/route.ts` (POST). + Both fixed in `1eb00389` by adding + `await requireAssistantSurface("coach")` after `requireAuth()`. +- **Invariants run green** — confirmed via the test gate runs above. + +## Commits + +- `1eb00389` — `test(coach): discovery test pins requireAssistantSurface on every coach insights route` (Item 3 + 2 missing-gate fixes) +- `5ecc3152` — `test(coach): make route-inventory gate check skip pure-comment lines` (self-review) + +Items 1 (H4) and 2 (M-5) were already in HEAD via `c72b3ce8`; no commits were authored by this wave for those items because the diffs were no-ops. diff --git a/.planning/phase-W-F-v1438-perf-hotspots-report.md b/.planning/phase-W-F-v1438-perf-hotspots-report.md new file mode 100644 index 000000000..db992ce63 --- /dev/null +++ b/.planning/phase-W-F-v1438-perf-hotspots-report.md @@ -0,0 +1,183 @@ +# v1.4.38 W-F — perf-hotspot fix-up + +Two endpoints surfaced as 3-5 s cold during v1.4.37.x verification. Same +shape as the v1.4.37.2 slim-summaries fix: in both cases the JS layer +was throwing away most of the row volume it asked for. Applied the same +SQL-aggregation playbook here, plus a 60 s LRU wrap on the iOS-only +dashboard route and per-sub-query timing annotations so the next +perf-verify can attribute regressions without re-instrumenting. + +## Endpoint A — `GET /api/dashboard/summary` + +### Before + +Four sub-queries inside the `Promise.all`, plus a fifth conditional +after it: + +| # | Query | Row count on a power-user account | +|---|-------|-----------------------------------| +| A1 | `measurement.findMany({ measuredAt: { gte: -7d } })` | thousands (Apple Health step samples) | +| A2 | `measurement.groupBy({ by: ["type"], _count, _max })` | ≤ N metric types (~10) | +| A3 | `medicationIntakeEvent.findMany({ scheduledFor: today })` | ≤ daily schedule count | +| A4 | `medicationIntakeEvent.findMany({ scheduledFor: { gte: -365d } })` | hundreds | +| A5 (conditional) | `measurement.findMany({ measuredAt: { gte: -365d } })` | up to **tens of thousands** of raw rows | + +Wall-clock: ~4.6 s cold mount per Marc's HAR capture. A1 and A5 were +dominant on power-user accounts. + +### After + +Six bounded sub-queries; the conditional fifth is folded back into the +Promise.all: + +| # | Query | Bounded row count | +|---|-------|-------------------| +| B1 | `$queryRaw DISTINCT ON (type)` over 7d window | one row per metric type | +| B2 | `$queryRaw` over `measurement_rollups` DAY buckets in 7d | ≤ `SPARK_DAYS × N` ≈ 70 | +| B3 | `measurement.groupBy({ by: ["type"], _count, _max })` | unchanged | +| B4 | `medicationIntakeEvent.findMany({ scheduledFor: today })` | unchanged | +| B5 | `medicationIntakeEvent.findMany({ scheduledFor: { gte: -365d } })` | unchanged | +| B6 | `$queryRaw SELECT DISTINCT to_char(measured_at AT TIME ZONE $tz, 'YYYY-MM-DD')` | ≤ 365 | + +Then wrap the whole response in `caches.analytics` keyed +`${userId}|dashboard-summary` with a 60 s TTL. The cache hit path skips +the whole builder. + +Behaviour notes: +- `sparkline` is now the DAY-bucket mean per day instead of every raw + reading. For BP/weight that's effectively the same shape because the + user logs ≤1 per day. For ACTIVITY_STEPS / sleep / glucose the + smoother per-day average is a better trend signal anyway. +- `latestValue` semantics preserved — still the most recent reading in + the trailing 7-day window. When the window is empty, `latestValue` + stays null and `lastSeenAt` falls through to the all-time `_max` + (unchanged behaviour). +- The `AT TIME ZONE $tz` conversion is done server-side so the day-key + set is byte-identical to what `userDayKey(measuredAt, userTz)` would + produce in JS. + +### Cache-invalidation hooks + +Extended `invalidateUserMedications` to flush `caches.analytics` for +the user prefix. Without this a medication intake event would not +invalidate the dashboard summary (measurement / mood writes already +flushed analytics via the existing helpers). + +### Estimated wall-clock + +| Path | Before | After | +|---|---|---| +| Cold mount, rollup-fresh | ~4.6 s | ~500 ms | +| Warm cache hit (within 60 s TTL) | n/a | < 50 ms (Map lookup) | +| Cold mount, rollup-empty (brand-new account) | ~4.6 s | ~600 ms (DAY-bucket query returns empty, sparkline degrades gracefully) | + +### Per-sub-query timing labels + +Emitted under `meta.dashboard.sub_