diff --git a/src/__tests__/workout-canonical-identity-guard.test.ts b/src/__tests__/workout-canonical-identity-guard.test.ts new file mode 100644 index 000000000..7c976d1ec --- /dev/null +++ b/src/__tests__/workout-canonical-identity-guard.test.ts @@ -0,0 +1,120 @@ +/** + * Structural guard on the canonical-workout session identity. + * + * `pickCanonicalWorkoutRows()` collapses a HealthKit session that arrived more + * than once under different external UUIDs. It can only do that when the row it + * is handed carries the identity fields, and the row shape comes from each + * caller's own Prisma `select`. Nothing in the type system connects the two: + * `durationSec` is optional on `WorkoutPickerRow`, so a caller that omits it + * gets silent passthrough rather than a compile error. + * + * That is exactly what happened. When the collapse first shipped, the key was + * `(sportType, startedAt, endedAt, durationSec)` and only the workout list + * selected `endedAt`. The list showed one workout while the Coach narrated the + * duplicate, the per-sport baseline averaged it twice and the deep-link cluster + * kept both rows. One record, four surfaces, two answers. + * + * These are tripwires, not proofs. They cannot show the collapse is correct — + * that is what the unit tests do. What they can do is refuse to let a new call + * site join the set without someone editing this file, and refuse to let the + * identity quietly grow a field that only some callers fetch. + */ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { globSync } from "node:fs"; +import { join, sep } from "node:path"; + +const SRC = join(process.cwd(), "src"); + +/** The picker module itself — declares the identity, does not consume it. */ +const PICKER = "lib/measurements/pick-canonical-workout-rows.ts"; + +/** + * Every production call site of the read-time picker, frozen. + * + * Adding a surface that collapses workouts is a deliberate act: it must fetch + * the identity fields or it silently disagrees with every other surface. Adding + * the file here is the acknowledgement. + */ +const IDENTITY_OWNER: Record = { + "app/api/workouts/[id]/route.ts": "app/api/workouts/[id]/route.ts", + "lib/jobs/workout-insight-generate.ts": + "lib/jobs/workout-insight-generate.ts", + "lib/workouts/list-read.ts": "lib/workouts/list-read.ts", + "lib/workouts/sport-context.ts": "lib/workouts/sport-context.ts", + // The only indirection in the set. The Coach block is handed rows it does not + // read itself; the snapshot builder fetches them in parallel with every other + // block's data. Recording the owner here rather than assuming the call site + // owns its own select is the whole point of this map. + "lib/ai/coach/snapshot-blocks/workouts-block.ts": "lib/ai/coach/snapshot.ts", +}; + +const EXPECTED_CALL_SITES = Object.keys(IDENTITY_OWNER).sort(); + +function sourceFiles(): string[] { + return globSync("**/*.{ts,tsx}", { cwd: SRC }) + .map((p) => p.split(sep).join("/")) + .filter((p) => !p.startsWith("generated/")) + .filter((p) => !p.includes("__tests__")) + .filter((p) => !p.endsWith(".test.ts") && !p.endsWith(".test.tsx")) + .sort(); +} + +function read(rel: string): string { + return readFileSync(join(SRC, rel), "utf8"); +} + +/** + * Lines that actually invoke the picker. Comment lines are excluded because the + * write-time sibling (`lib/workouts/canonical-rows.ts`) names the read-time + * picker in its docblock to explain how the two layer, and that mention is + * documentation rather than a call. + */ +function invokesPicker(source: string): boolean { + return source + .split("\n") + .some( + (line) => + line.includes("pickCanonicalWorkoutRows(") && + !/^\s*(\*|\/\/|\/\*)/.test(line), + ); +} + +describe("the picker's call-site set is frozen", () => { + it("has exactly the call sites this file acknowledges", () => { + const found = sourceFiles() + .filter((rel) => rel !== PICKER) + .filter((rel) => invokesPicker(read(rel))); + + expect(found).toEqual(EXPECTED_CALL_SITES); + }); +}); + +describe("every call site fetches the session identity", () => { + it.each(EXPECTED_CALL_SITES)("%s gets rows carrying durationSec", (rel) => { + // File-level rather than per-select: a caller that fetches workout rows + // without their duration cannot collapse a re-send, whichever select the + // rows came from. Coarse in the safe direction — it can only fail to catch + // a second unrelated select in the same file, never pass a caller that + // fetches no duration at all. + expect(read(IDENTITY_OWNER[rel])).toContain("durationSec: true"); + }); +}); + +describe("the identity cannot quietly grow a field callers do not fetch", () => { + it("keys the session on sportType, startedAt and durationSec only", () => { + const picker = read(PICKER); + const body = picker.slice( + picker.indexOf("function exactSessionKey"), + picker.indexOf("function rowRichness"), + ); + + expect(body).toContain("row.sportType"); + expect(body).toContain("row.startedAt"); + expect(body).toContain("row.durationSec"); + // `endedAt` is the specific field that split the callers. On a workout with + // pauses it disagrees with `durationSec` by design, so it rejected genuine + // re-sends while forcing every caller to fetch a fourth column. + expect(body).not.toContain("endedAt"); + }); +}); diff --git a/src/app/api/workouts/[id]/route.ts b/src/app/api/workouts/[id]/route.ts index 97fd208d5..439dbe5f2 100644 --- a/src/app/api/workouts/[id]/route.ts +++ b/src/app/api/workouts/[id]/route.ts @@ -114,6 +114,11 @@ export const GET = apiHandler( source: true, startedAt: true, sportType: true, + // Part of the canonical session identity consulted by + // `pickCanonicalWorkoutRows` below — without it a HealthKit re-send of + // this very workout stays in the cluster and the deep link can resolve + // to the duplicate instead of the row the list shows. + durationSec: true, avgHeartRate: true, maxHeartRate: true, metadata: true, diff --git a/src/lib/measurements/__tests__/pick-canonical-workout-rows.test.ts b/src/lib/measurements/__tests__/pick-canonical-workout-rows.test.ts index be2f4f13d..29e43e414 100644 --- a/src/lib/measurements/__tests__/pick-canonical-workout-rows.test.ts +++ b/src/lib/measurements/__tests__/pick-canonical-workout-rows.test.ts @@ -277,6 +277,68 @@ describe("pickCanonicalWorkoutRows", () => { expect(result[0].id).toBe("duplicate-b"); }); + it("collapses a re-send whose endedAt disagrees with the first arrival", () => { + // A workout with pauses has a wall-clock end that is longer than the + // counted duration, and the two sources of that number do not always agree + // across re-sends. `endedAt` was part of the session key when the collapse + // first shipped, so this exact pair survived as two rows. + const base = { + startedAt: new Date("2026-07-29T12:54:00Z"), + durationSec: 480, + sportType: "cycling", + source: "APPLE_HEALTH" as const, + }; + const result = pickCanonicalWorkoutRows([ + { + ...base, + id: "first-arrival", + endedAt: new Date("2026-07-29T13:02:00Z"), + }, + { + ...base, + id: "resend", + endedAt: new Date("2026-07-29T13:09:00Z"), + avgHeartRate: 132, + }, + ]); + + expect(result).toHaveLength(1); + expect(result[0].id).toBe("resend"); + }); + + it("keeps two same-source sessions whose durations differ", () => { + // The counterpart guarantee: the looser key must not merge genuinely + // distinct sessions that happen to share a bucket slot and a sport. + const base = { + startedAt: new Date("2026-07-29T12:54:00Z"), + sportType: "strength", + source: "APPLE_HEALTH" as const, + }; + const result = pickCanonicalWorkoutRows([ + { ...base, id: "short", durationSec: 480 }, + { ...base, id: "long", durationSec: 1800 }, + ]); + + expect(result.map((row) => row.id).sort()).toEqual(["long", "short"]); + }); + + it("passes rows without a duration through rather than guessing", () => { + // Fixture-only path: a row with no duration carries no session identity, so + // it cannot be collapsed. Every production caller selects the field and + // `workout-canonical-identity-guard.test.ts` keeps that true. + const base = { + startedAt: new Date("2026-07-29T12:54:00Z"), + sportType: "yoga", + source: "APPLE_HEALTH" as const, + }; + const result = pickCanonicalWorkoutRows([ + { ...base, id: "no-duration-a" }, + { ...base, id: "no-duration-b" }, + ]); + + expect(result).toHaveLength(2); + }); + it("does not collapse or enrich rows belonging to different users", () => { const result = pickCanonicalWorkoutRows([ { diff --git a/src/lib/measurements/pick-canonical-workout-rows.ts b/src/lib/measurements/pick-canonical-workout-rows.ts index 31fe6dce2..cb97fd0ad 100644 --- a/src/lib/measurements/pick-canonical-workout-rows.ts +++ b/src/lib/measurements/pick-canonical-workout-rows.ts @@ -46,8 +46,12 @@ import { export interface WorkoutPickerRow { userId?: string; startedAt: Date; - /** Optional exact-session fields used to collapse repeated same-source sync rows. */ - endedAt?: Date; + /** + * Session length. Together with `sportType` + `startedAt` this is the + * exact-session identity used to collapse repeated same-source sync rows. + * Optional only so fixtures can omit it; every production call site MUST + * select it, and `workout-canonical-identity-guard.test.ts` freezes that set. + */ durationSec?: number; sportType: string; source: MeasurementSource; @@ -60,18 +64,31 @@ export interface WorkoutPickerRow { * HealthKit can resend the same session with a new external UUID. The source * ladder correctly chooses Apple over WHOOP, but previously kept every Apple * row in that winning bucket, which made the web list show the same workout - * two or three times. Only collapse rows when the exact session identity is - * available; fixtures and legacy callers without end/duration remain - * governed by the original bucket contract. + * two or three times. + * + * The identity is `(sportType, startedAt, durationSec)`. `endedAt` was part of + * this key when the collapse first shipped and has been dropped deliberately: + * + * - It is not a free extra check. On a workout with pauses, `endedAt` is the + * wall-clock end while `durationSec` counts only moving time, so the two + * legitimately disagree, and a stored row can carry either convention. Two + * re-sends of one session can therefore differ in `endedAt` alone, and the + * stricter key let that duplicate through. + * - It bought nothing in exchange. Nobody starts a second workout of the + * same sport in the same instant with the same duration, so start plus + * duration already identifies the session uniquely. + * - Requiring it split the callers. Four of the five call sites select + * `durationSec` but not `endedAt`, so the collapse ran on the workout list + * and silently did nothing for the Coach narration, the per-sport baseline + * and the deep-link cluster. One list, one number, two answers. + * + * A row without `durationSec` cannot be identified and passes through + * uncollapsed. That is a fixture-only path: every production call site selects + * the field and `workout-canonical-identity-guard.test.ts` keeps it that way. */ function exactSessionKey(row: WorkoutPickerRow): string | null { - if (row.endedAt == null || row.durationSec == null) return null; - return [ - row.sportType, - row.startedAt.getTime(), - row.endedAt.getTime(), - row.durationSec, - ].join(":"); + if (row.durationSec == null) return null; + return [row.sportType, row.startedAt.getTime(), row.durationSec].join(":"); } function rowRichness(row: WorkoutPickerRow): number {