fix(coach,mcp): collapse workout dedup and union HRV flavours for MCP parity

The Coach snapshot read raw prisma.workout rows while GET /api/workouts
runs pickCanonicalWorkoutRows, so a dual-source user (e.g. Apple Watch +
Withings recording the same run) got the session double-counted in the
Coach prompt and the get_workouts MCP tool. Route the snapshot's workout
block through the same picker, keyed off the user's own sourcePriorityJson.

HRV resolves SDNN only on every MCP/coach path, so an Oura/Polar/WHOOP-only
account (RMSSD, not SDNN) saw a false empty HRV block even though the app's
HRV sub-page already unions both flavours. The coach snapshot now queries
both types; the MCP rich reads (compare_metric, get_metric_baseline,
detect_changepoints) fall back to RMSSD when the user has zero SDNN rows,
mirroring the app's primary-then-fallback rule.

Also aligns surfaces.mcp as a real gate: a metric-status id the signal
registry marks mcp:false (cardio recovery, wrist temperature, sleep score,
...) no longer resolves through the rich-read registry path, closing a
contract gap between the two registries (no data was actually exposed
before this, but a future mcp:false signal without this guard would have
leaked silently).

The remaining day-edge tz mismatch between the rollup-backed rich reads
and the user-tz coach timelines is left documented rather than patched:
the underlying rollup buckets are themselves written on UTC calendar
boundaries, so shifting only the read-side window would trade one
one-sided inconsistency for another without fixing the root cause.
This commit is contained in:
Marc-André Bombeck committed 2026-07-18 07:56:14 +02:00
1 parent 6b259c2c28
commit c6885bc7d3
5 files changed
+319 -19

No files matched your search

@@ -123,6 +123,26 @@ describe("buildCoachSnapshot — Apple Health additive metrics (v1.4.23)", () =>
expect(out.provenance.counts?.hrv).toBe(2);
});
// v1.30.4 (G4/HRV union) — Oura / Polar / WHOOP write nightly HRV as RMSSD
// (`HRV_RMSSD`) only, never SDNN (`HEART_RATE_VARIABILITY`). Before this
// fix the snapshot's `hrv` source only queried SDNN, so a ring/strap-only
// account got a false empty hrv block even though the app's HRV sub-page
// charts the RMSSD rows. Pin the RMSSD-only path resolving.
it("emits the hrv block from HRV_RMSSD rows alone (ring/strap-only account)", async () => {
const rows = [daysAgo(2, 38, "HRV_RMSSD"), daysAgo(5, 41, "HRV_RMSSD")];
prismaMock.measurement.findMany.mockResolvedValue(rows);
const out = await buildCoachSnapshot("user-1", {
sources: ["hrv"],
window: "last30days",
});
const snapshot = JSON.parse(out.snapshotJson) as Record<string, unknown>;
expect(snapshot.heartRateVariability).toBeDefined();
expect(out.provenance.metrics).toContain("hrv");
expect(out.provenance.counts?.hrv).toBe(2);
});
it("emits a sleep timeline block on SLEEP_DURATION rows", async () => {
const rows = [
daysAgo(1, 420, "SLEEP_DURATION"),
@@ -339,6 +359,64 @@ describe("buildCoachSnapshot — Apple Health additive metrics (v1.4.23)", () =>
expect(out.provenance.metrics).toContain("workouts");
});
// v1.30.4 (C1) — `GET /api/workouts` collapses a dual-source recording of
// the SAME session (e.g. Apple Watch + Withings both logging one run) with
// `pickCanonicalWorkoutRows` before the UI ever counts it. The Coach
// snapshot (and the `get_workouts` MCP tool, which re-exports this same
// block) used to read `prisma.workout.findMany` raw with no read-time
// collapse, so a dual-device user's Coach prompt double-counted the
// session and inflated `perSport`/`totalInWindow` vs. the app. Pin the fix.
it("collapses a same-session cross-source workout pair to one, matching the UI", async () => {
const sharedStart = new Date("2026-06-03T06:30:00Z");
const workouts = [
{
sportType: "RUNNING",
startedAt: sharedStart,
durationSec: 1800,
totalEnergyKcal: 400,
totalDistanceM: 5000,
avgHeartRate: 140,
maxHeartRate: 175,
source: "APPLE_HEALTH",
},
{
sportType: "RUNNING",
// 90 s apart — same 5-minute bucket the picker uses.
startedAt: new Date(sharedStart.getTime() + 90_000),
durationSec: 1780,
totalEnergyKcal: 390,
totalDistanceM: 4950,
avgHeartRate: 138,
maxHeartRate: 172,
source: "WITHINGS",
},
];
prismaMock.workout.findMany.mockResolvedValue(workouts);
const out = await buildCoachSnapshot("user-1", {
sources: ["workouts"],
window: "last30days",
});
const snapshot = JSON.parse(out.snapshotJson) as {
workouts?: {
recent?: Array<{ sport: string }>;
perSport?: Array<{ sport: string; count: number }>;
totalInWindow?: number;
};
};
// Collapsed to the one Apple Health row (default ladder: Apple Health
// outranks Withings) — never two.
expect(snapshot.workouts?.totalInWindow).toBe(1);
expect(snapshot.workouts?.recent?.length).toBe(1);
expect(snapshot.workouts?.perSport).toEqual([
{
sport: "RUNNING",
count: 1,
totalDurationMin: 30,
totalEnergyKcal: 400,
},
]);
});
it("subtracts an excluded metric inside an otherwise-enabled cluster", async () => {
// The user enabled the body cluster (default) but excluded weight.
// Weight rows must not produce a block even though the cluster is on.
@@ -10,7 +10,18 @@
* Split out of `snapshot.ts`; the builder passes the rows it already
* read plus the shared accumulators, so the emitted shape and ordering
* are unchanged.
*
* v1.30.4 (C1) — `GET /api/workouts` collapses cross-source duplicate
* sessions (e.g. Apple Watch + WHOOP recording the same run) with
* `pickCanonicalWorkoutRows()` before the UI ever sees them; this block
* read `prisma.workout.findMany` raw, so a dual-source user's Coach
* prompt (and the `get_workouts` MCP tool, which re-exports this same
* block) double-counted sessions and inflated the per-sport rollup vs.
* the UI. Run the SAME picker here, keyed off the user's own
* `sourcePriorityJson`, so both surfaces agree.
*/
import { pickCanonicalWorkoutRows } from "@/lib/measurements/pick-canonical-workout-rows";
import type { MeasurementSource } from "@/generated/prisma/client";
import { annotate } from "@/lib/logging/context";
import { tzDayKey, tzWeekday } from "../snapshot-series";
import type {
@@ -35,7 +46,12 @@ interface WorkoutsBlockContext {
totalDistanceM: number | null;
avgHeartRate: number | null;
maxHeartRate: number | null;
source: MeasurementSource;
}>;
// v1.30.4 (C1) — the user's own source-priority blob, same shape
// `pickCanonicalWorkoutRows` and the sleep block already consume.
// `null` falls back to the canonical default ladder.
sourcePriorityJson: unknown;
userTz: string;
snapshot: Record<string, unknown>;
metrics: Set<CoachProvenanceMetric>;
@@ -44,8 +60,26 @@ interface WorkoutsBlockContext {
}
export function buildWorkoutsBlock(ctx: Readonly<WorkoutsBlockContext>): void {
const { workoutRows, userTz, snapshot, metrics, counts, registerBlock } = ctx;
// The workout sessions are read in parallel by the builder.
const {
workoutRows: rawWorkoutRows,
sourcePriorityJson,
userTz,
snapshot,
metrics,
counts,
registerBlock,
} = ctx;
// The workout sessions are read in parallel by the builder, raw
// (every source, every duplicate). Collapse cross-source duplicates
// with the same picker the UI uses before narrating anything.
// `pickCanonicalWorkoutRows` always returns its output sorted
// `startedAt` ASC (its documented contract) — the builder wants
// most-recent-first for the capped `recent` list, so re-establish
// DESC order after the pick, same as `GET /api/workouts` does.
const workoutRows = pickCanonicalWorkoutRows(
rawWorkoutRows,
sourcePriorityJson ?? null,
).sort((a, b) => b.startedAt.getTime() - a.startedAt.getTime());
if (workoutRows.length === 0) {
annotate({
action: { name: "coach.cluster.empty_skipped" },
+33 -7
View File
@@ -591,7 +591,13 @@ async function buildCoachSnapshotImpl(
pulse: ["PULSE"],
mood: [],
compliance: [],
hrv: ["HEART_RATE_VARIABILITY"],
// v1.30.4 (HRV union) — Oura / Polar / WHOOP write nightly HRV as RMSSD
// (`HRV_RMSSD`), never SDNN (`HEART_RATE_VARIABILITY`, Apple / Fitbit).
// The app's HRV surface already unions both (`sub-page-metric.ts`);
// resolving SDNN alone here made a ring/strap-only user's Coach context
// + the `get_metric_series`/rich-read MCP paths report a false
// `{present:false}` for HRV even though the app charts it.
hrv: ["HEART_RATE_VARIABILITY", "HRV_RMSSD"],
sleep: ["SLEEP_DURATION"],
resting_hr: ["RESTING_HEART_RATE"],
steps: ["ACTIVITY_STEPS"],
@@ -843,6 +849,10 @@ async function buildCoachSnapshotImpl(
totalDistanceM: true,
avgHeartRate: true,
maxHeartRate: true,
// v1.30.4 (C1) — needed by `pickCanonicalWorkoutRows` in the block
// builder to collapse a dual-source session (e.g. Apple Watch +
// WHOOP recording the same run) to one, matching `GET /api/workouts`.
source: true,
},
})
: null;
@@ -974,13 +984,19 @@ async function buildCoachSnapshotImpl(
labsBlockPromise,
]);
const byType = (t: string) =>
measurementRows
.filter((r) => r.type === t)
// v1.30.4 (HRV union) — accepts either a single `MeasurementType` or a
// union of types (e.g. HRV's SDNN + RMSSD flavours) so a caller can
// resolve "whichever the user actually has" the same way the app's HRV
// sub-page does, without a second query.
const byType = (t: string | readonly string[]) => {
const wanted = Array.isArray(t) ? new Set(t) : null;
return measurementRows
.filter((r) => (wanted ? wanted.has(r.type) : r.type === t))
.map((r) => ({
measuredAt: r.measuredAt,
value: r.value,
}));
};
if (wantsBp && features.bloodPressure) {
const sysRows = byType("BLOOD_PRESSURE_SYS");
@@ -1132,7 +1148,9 @@ async function buildCoachSnapshotImpl(
metric: ValueMetric;
source: CoachScopeSource;
snapshotKey: string;
type: string;
// v1.30.4 (HRV union) — most metrics are a single `MeasurementType`;
// hrv resolves a union of SDNN + RMSSD (see `byType` above).
type: string | readonly string[];
};
// v1.4.23 W6 (S-04) / v1.7.0 — every single-`MeasurementType`
// additive series ships as a timeline-only block (recent day rows +
@@ -1149,7 +1167,7 @@ async function buildCoachSnapshotImpl(
metric: "hrv",
source: "hrv",
snapshotKey: "heartRateVariability",
type: "HEART_RATE_VARIABILITY",
type: ["HEART_RATE_VARIABILITY", "HRV_RMSSD"],
},
{
metric: "resting_hr",
@@ -1370,7 +1388,14 @@ async function buildCoachSnapshotImpl(
registerBlock(block.snapshotKey, block.source);
// W7 grounding: when this additive series maps to a reference metric,
// record the recent daily mean (same per-day means the timeline shows).
const refMetric = TYPE_TO_REFERENCE_METRIC[block.type];
// Union-type blocks (hrv) never carry a reference-metric grounding
// entry today, so the lookup only fires for a plain string type.
// (`typeof === "string"` narrows cleanly here; `Array.isArray` does not
// exclude a `readonly string[]` arm from the `string` union.)
const refMetric =
typeof block.type === "string"
? TYPE_TO_REFERENCE_METRIC[block.type]
: undefined;
if (refMetric) {
const recentRows = buildDailyValueRows(rows, recentCutoff, userTz);
const mean = recentRowsMean(recentRows);
@@ -1436,6 +1461,7 @@ async function buildCoachSnapshotImpl(
if (sources.has("workouts") && workoutRows) {
buildWorkoutsBlock({
workoutRows,
sourcePriorityJson: prefsRow?.sourcePriorityJson ?? null,
userTz,
snapshot,
metrics,
+79 -2
View File
@@ -15,10 +15,14 @@ vi.mock("@/lib/rollups/measurement-read-wmy", async (importOriginal) => {
await importOriginal<typeof import("@/lib/rollups/measurement-read-wmy")>();
return { ...actual, readBestGranularityRollups: vi.fn() };
});
const { labResult } = vi.hoisted(() => ({
const { labResult, measurement } = vi.hoisted(() => ({
labResult: { findMany: vi.fn() },
// v1.30.4 (G4) — `withHrvFallback`'s presence probe. Unused by every
// metric that doesn't carry a `fallbackMeasurementType` (everything but
// HRV today); those tests never touch this mock.
measurement: { count: vi.fn() },
}));
vi.mock("@/lib/db", () => ({ prisma: { labResult } }));
vi.mock("@/lib/db", () => ({ prisma: { labResult, measurement } }));
import {
getCorrelation,
@@ -124,6 +128,28 @@ describe("resolveRichMetric", () => {
expect(resolveRichMetric(off)).toBeNull();
}
});
// v1.30.4 (C2) — the signal registry's `surfaces.mcp` flag is documented as
// the SINGLE source of truth for MCP exposure, but `resolveRichMetric`'s
// metric-status-registry path (exact id / display-name match) used to
// resolve an id regardless of that flag. `CARDIO_RECOVERY` /
// `WRIST_TEMPERATURE` / `SLEEP_SCORE` are all valid `METRIC_STATUS_IDS`
// entries but carry `mcp:false` in the signal registry — pin that they
// now stay unreachable, closing the contract gap (no PHI actually leaked
// through it, but a future `mcp:false` signal without this guard would).
it("honours the signal registry's mcp:false as a hard veto over a metric-status hit", () => {
expect(resolveRichMetric("CARDIO_RECOVERY")).toBeNull();
expect(resolveRichMetric("cardio recovery")).toBeNull();
expect(resolveRichMetric("WRIST_TEMPERATURE")).toBeNull();
expect(resolveRichMetric("SLEEP_SCORE")).toBeNull();
});
it("still resolves a metric-status id the signal registry does NOT mark mcp:false", () => {
// VO2 max also has a signal-registry entry, but it's `mcp: true` there —
// the C2 guard only vetoes an EXPLICIT `mcp:false`, so this stays
// resolvable exactly as before.
expect(resolveRichMetric("vo2_max")?.measurementType).toBe("VO2_MAX");
});
});
// ── 1. get_correlation ───────────────────────────────────────────────
@@ -321,6 +347,57 @@ describe("get_metric_baseline", () => {
expect(res.present).toBe(false);
expect(res.reason).toBe("unknown_metric");
});
// v1.30.4 (G4/HRV union) — Oura / Polar / WHOOP write nightly HRV as RMSSD
// (`HRV_RMSSD`) only, never SDNN (`HEART_RATE_VARIABILITY`). Before this
// fix `resolveRichMetric("hrv")` always read SDNN, so a ring/strap-only
// account got a false `{present:false}` even though the app's HRV
// sub-page charts the RMSSD rows. Pin the RMSSD-only path resolving.
it("resolves hrv from HRV_RMSSD rows when the user has zero SDNN rows", async () => {
measurement.count.mockImplementation(
async ({ where }: { where: { type: string } }) =>
where.type === "HEART_RATE_VARIABILITY" ? 0 : 6,
);
vi.mocked(buildCoachReadStrip).mockResolvedValue({
baseline: {
low: 30,
high: 45,
latest: 38,
placement: "within",
sampleDays: 30,
},
learning: false,
driver: null,
});
const res = await getMetricBaseline(USER, { metric: "hrv" });
expect(res.present).toBe(true);
expect(buildCoachReadStrip).toHaveBeenCalledWith(USER, "HRV_RMSSD");
});
it("stays on SDNN when the user has any HEART_RATE_VARIABILITY rows", async () => {
measurement.count.mockResolvedValue(3); // primary count > 0, short-circuits
vi.mocked(buildCoachReadStrip).mockResolvedValue({
baseline: {
low: 55,
high: 65,
latest: 60,
placement: "within",
sampleDays: 30,
},
learning: false,
driver: null,
});
const res = await getMetricBaseline(USER, { metric: "hrv" });
expect(res.present).toBe(true);
expect(buildCoachReadStrip).toHaveBeenCalledWith(
USER,
"HEART_RATE_VARIABILITY",
);
});
});
// ── 4. detect_changepoints ───────────────────────────────────────────
+93 -8
View File
@@ -96,6 +96,28 @@ const WINDOW_DAYS: Record<CoachScopeWindow, number> = {
allTime: 365,
};
// v1.30.4 (C3, documented not fixed) — the trailing windows above resolve to
// `since = now − N·86400_000ms` (`readBestGranularityRollups`,
// `measurement-read-wmy.ts`), a raw-instant cutoff with no timezone
// awareness. The Coach snapshot path (`get_metric_series`) builds its
// timelines on user-tz day keys (`tzDayKey(userTz)`, the v1.30.3 pattern also
// used by `metric-status.ts`'s cache rollover / `bp-in-target.ts`'s same-day
// pairing); these rich reads (`compare_metric` / `detect_changepoints` /
// `get_metric_baseline`) do not, so a user far from UTC can see a ±1-day edge
// wobble vs. the app for a window boundary that lands mid-day in their own
// tz. This is INTENTIONALLY left as a raw-instant cutoff rather than patched
// with a tz-shifted `since` here: the underlying `MeasurementRollup` DAY /
// WEEK / MONTH / YEAR buckets themselves are written on UTC calendar
// boundaries (`measurement-rollups.ts` buckets on `Date.UTC(...)` /
// `getUTCDay()`), not the user's own tz, so shifting only the outer window
// boundary would not eliminate the wobble — it would just move which
// UTC-anchored bucket sits at the edge, trading one one-sided inconsistency
// for another. A real fix needs the rollup tier itself to bucket per-user-tz,
// which is a write-side change (affecting every rollup consumer, not just
// the rich MCP reads) — out of scope here. Practical impact stays small: the
// exposed windows are all ≥7-day means, so a single boundary day shifts the
// mean by well under its own noise floor.
function windowToDays(window: CoachScopeWindow | undefined): number {
return WINDOW_DAYS[window ?? "last30days"];
}
@@ -117,6 +139,17 @@ export interface RichMetric {
unit: string;
/** Population reference band, or `null` when no universal band exists. */
band: { low: number; high: number } | null;
/**
* v1.30.4 (G4/HRV union) — a secondary `MeasurementType` to read instead of
* `measurementType` when the user has ZERO rows of the primary type. HRV is
* the only metric that carries one today: Apple Health / Fitbit write SDNN
* (`HEART_RATE_VARIABILITY`); Oura / Polar / WHOOP write nightly RMSSD
* (`HRV_RMSSD`) only. Mirrors the app's HRV sub-page fallback
* (`HealthKitMetricPage`'s `fallbackMeasurementType` prop) so a ring/strap
* -only account resolves over MCP the same data it sees in the app instead
* of a false `{ present: false }`.
*/
fallbackMeasurementType?: MeasurementType;
}
/** Headline specialised metrics the status registry does not carry. */
@@ -168,7 +201,24 @@ const ALIASES: Record<string, string> = {
function fromRegistry(id: string): RichMetric | null {
const meta = getMetricStatusMeta(id);
if (!meta) return null;
return {
// v1.30.4 (C2) — the signal registry's `surfaces.mcp` flag is documented as
// the SINGLE source of truth for MCP exposure, but the metric-status
// registry (this function's own source) carries no `surfaces` facet of its
// own, so an id the signal registry marks `mcp:false` (e.g.
// `CARDIO_RECOVERY`, `WRIST_TEMPERATURE`, `SLEEP_SCORE`, `DAY_STRAIN`, the
// stair speeds, …) still resolved here — the two registries disagreeing
// about the exposure contract, not a live leak (none of those ids are
// actually sensitive) but a future signal added `mcp:false` + a
// metric-status entry would leak silently. Treat an EXPLICIT `mcp:false` on
// the SAME id in the signal registry as a hard veto over a metric-status
// hit; an id absent from the signal registry (most of `METRIC_STATUS_IDS`)
// is unaffected — metric-status stays authoritative when the two don't
// disagree.
const sig = getSignal(id);
if (sig && sig.kind === "measurement" && sig.surfaces.mcp === false) {
return null;
}
const metric: RichMetric = {
measurementType: meta.measurementType,
label: meta.displayName,
unit: meta.unit,
@@ -176,6 +226,38 @@ function fromRegistry(id: string): RichMetric | null {
? { low: meta.normalRange.low, high: meta.normalRange.high }
: null,
};
// v1.30.4 (G4/HRV union) — single choke point every registry-backed
// resolution path (alias / exact id / display-name match) flows through,
// so the RMSSD fallback attaches uniformly regardless of how the caller
// phrased the metric.
if (metric.measurementType === "HEART_RATE_VARIABILITY") {
return { ...metric, fallbackMeasurementType: "HRV_RMSSD" };
}
return metric;
}
/**
* v1.30.4 (G4/HRV union) — swap `metric` to its `fallbackMeasurementType`
* when the user has zero rows of the primary type but has rows of the
* fallback. Same primary-then-fallback rule the HRV sub-page applies
* (`usingFallback = primaryCount === 0 && fallbackCount > 0`); a no-op for
* every metric that doesn't carry a fallback (i.e. everything but HRV
* today), and for an HRV account that has SDNN data at all.
*/
async function withHrvFallback(
userId: string,
metric: RichMetric,
): Promise<RichMetric> {
if (!metric.fallbackMeasurementType) return metric;
const primaryCount = await prisma.measurement.count({
where: { userId, type: metric.measurementType, deletedAt: null },
});
if (primaryCount > 0) return metric;
const fallbackCount = await prisma.measurement.count({
where: { userId, type: metric.fallbackMeasurementType, deletedAt: null },
});
if (fallbackCount === 0) return metric;
return { ...metric, measurementType: metric.fallbackMeasurementType };
}
// ── v1.25 clinical signals (registry-grounded, MCP-owned exposure) ────
@@ -577,10 +659,11 @@ export async function compareMetric(
rangeB?: DateRange;
},
): Promise<CompareMetricResult> {
const metricA = resolveRichMetric(args.metric);
if (!metricA) {
const resolvedA = resolveRichMetric(args.metric);
if (!resolvedA) {
return { present: false, reason: "unknown_metric" };
}
const metricA = await withHrvFallback(userId, resolvedA);
const sideA: SideSpec = { window: args.window, range: args.range };
const annotateMiss = () =>
@@ -601,7 +684,7 @@ export async function compareMetric(
}
// Both metrics share side A's horizon (window or range).
mode = "metric_vs_metric";
metricB = resolvedB;
metricB = await withHrvFallback(userId, resolvedB);
sideB = sideA;
} else if (args.rangeB || args.windowB) {
mode = "window_vs_window";
@@ -675,10 +758,11 @@ export async function getMetricBaseline(
userId: string,
args: { metric: string },
): Promise<MetricBaselineResult> {
const metric = resolveRichMetric(args.metric);
if (!metric) {
const resolved = resolveRichMetric(args.metric);
if (!resolved) {
return { present: false, reason: "unknown_metric" };
}
const metric = await withHrvFallback(userId, resolved);
const strip = await buildCoachReadStrip(userId, metric.measurementType);
@@ -819,10 +903,11 @@ export async function detectChangepoints(
userId: string,
args: { metric: string; window?: CoachScopeWindow; range?: DateRange },
): Promise<ChangepointsResult> {
const metric = resolveRichMetric(args.metric);
if (!metric) {
const resolved = resolveRichMetric(args.metric);
if (!resolved) {
return { present: false, reason: "unknown_metric" };
}
const metric = await withHrvFallback(userId, resolved);
// Either an explicit `{from,to}` range (reach-back + filter) or one of the
// five fixed trailing windows — both land on the same rollup reader.