From 3406ed6946d6246b62ad27a585e902f7dd228d8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 31 Jul 2026 16:52:22 +0200 Subject: [PATCH] Drop a boundary condition no input could reach The recipe boundary also required a version of at least one before it would suppress. That reads like a second lock and is not one: the resolver returns a version below one only on the same path that returns no change date, so nothing could ever reach the condition and no test could make it fail. Removing it leaves the guard asking the one question that has an honest answer, which is whether the recipe moved inside the window. The invariant that makes the removal safe is now pinned instead: a resolved configuration never carries a version without the date beside it. A resolver path that produced one would wave that account's recipe change straight past the guard. Also pins the order of the two rules that can fire together. A change that moved the composition trips both, and the person is told they changed what counts rather than that the included pillars changed, which would describe the consequence and leave out the act. --- .../__tests__/config-delta-guard.test.ts | 75 +++++++++++++++++++ src/lib/analytics/score/composite.ts | 21 ++++-- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/lib/analytics/score/__tests__/config-delta-guard.test.ts b/src/lib/analytics/score/__tests__/config-delta-guard.test.ts index b0794076b..4eff8ddfc 100644 --- a/src/lib/analytics/score/__tests__/config-delta-guard.test.ts +++ b/src/lib/analytics/score/__tests__/config-delta-guard.test.ts @@ -176,6 +176,52 @@ describe("the recipe boundary", () => { expect(report.delta).toBe(-20); }); + it("names the act rather than its consequence when both would fire", () => { + // A change that also moved the composition trips two rules at once. + // The order matters to the sentence a person reads: "the included + // pillars changed" describes what happened to the number and leaves + // out who did it, which is the whole confusion this guard exists to + // remove. + const narrower = buildOk({ + value: { + score: 60, + band: "green", + bandSetter: null, + composition: ["BLOOD_PRESSURE", "ACTIVITY"], + noiseFloor: 1, + scoreVersion: 2, + }, + coverage: { + requiredInputs: 3, + presentInputs: 2, + historyDays: 28, + missing: [], + }, + confidence: { score: 80, band: "high" }, + provenance: { + inputs: ["BLOOD_PRESSURE", "ACTIVITY"], + source: "live", + windowDays: 28, + computedAt: NOW.toISOString(), + }, + }); + + const report = attachScoreDelta( + narrower, + composite(80, new Date(NOW.getTime() - 7 * DAY_MS)), + composite(80, new Date(NOW.getTime() - 14 * DAY_MS)), + NOW, + { + version: 2, + changedAt: new Date(NOW.getTime() - 3 * DAY_MS).toISOString(), + }, + CURRENT_PILLARS, + EARLIER_PILLARS, + ); + + expect(report.deltaReason).toBe("config_changed"); + }); + it("suppresses at the boundary instant and not one moment past it", () => { const previousAt = new Date(NOW.getTime() - 7 * DAY_MS); const boundary = (changedAt: Date) => @@ -212,6 +258,35 @@ describe("the recipe boundary", () => { ).toBe(false); }); + it("never resolves a recipe version without the date the guard reads", () => { + // The invariant the boundary rests on. `crossesConfigBoundary` asks + // only about the date, which is safe exactly because a resolved + // config never carries a version without one — an authored recipe + // always has both, and a never-authored one has neither. If a + // resolver path ever produced a version with no date, the guard + // would wave that account's recipe change straight through. + const blobs: unknown[] = [ + null, + undefined, + "not an object", + {}, + { excludedPillars: [] }, + { excludedPillars: ["SLEEP"] }, + { excludedPillars: ["SLEEP"], version: 2 }, + { excludedPillars: ["nonsense"], version: 9 }, + { excludedPillars: [], changedAt: "2026-08-01T00:00:00.000Z" }, + { version: 4, changedAt: "2026-08-01T00:00:00.000Z" }, + ]; + + for (const blob of blobs) { + const resolved = resolveHealthScoreConfig(blob); + expect( + resolved.version >= 1 || resolved.changedAt === null, + `version without a change date for ${JSON.stringify(blob)}`, + ).toBe(true); + } + }); + it("carries the resolver's own answer, not a second reading of the blob", () => { const changedAt = new Date(NOW.getTime() - 2 * DAY_MS); const stored = healthScoreConfigFromSelection({ diff --git a/src/lib/analytics/score/composite.ts b/src/lib/analytics/score/composite.ts index edc42e7e1..dd2b61cda 100644 --- a/src/lib/analytics/score/composite.ts +++ b/src/lib/analytics/score/composite.ts @@ -222,18 +222,27 @@ function comparableDynamicPillars( * subtraction looks legitimate. Nothing inside the two composites can * see the change; only the recipe's own `changedAt` can. * - * `version` is what says a recipe was ever authored. An account that - * never chose has no boundary to straddle, and a stored blob that - * somehow carries no `changedAt` gives nothing to compare, so neither - * suppresses: refusing every delta forever on an unreadable date would - * trade one silent lie for a permanent silence. + * The date is the whole signal, and the boundary's `version` is + * deliberately NOT consulted here. An earlier draft also required + * `version >= 1`, which reads like defence in depth and is not: the + * resolver hands back a version below 1 only on the path that also + * hands back a null date, so no input could ever reach that arm and no + * test could make it fail. A condition nothing can trip is decoration + * on a guard. The version earns its keep elsewhere — it identifies the + * recipe on the stored row and in the notice key — but the question + * "did the recipe move inside this window" has one honest answer and it + * is a date. + * + * No date means nothing to compare, and that does not suppress: + * refusing every delta forever over an unreadable timestamp would trade + * one silent lie for a permanent silence. */ export function crossesConfigBoundary( boundary: ScoreConfigBoundary, previousAt: Date, asOf: Date, ): boolean { - if (boundary.version < 1 || boundary.changedAt === null) return false; + if (boundary.changedAt === null) return false; const changedAt = new Date(boundary.changedAt); if (Number.isNaN(changedAt.getTime())) return false; return asOf >= changedAt && previousAt < changedAt;