From 59108e09fac4c24a7326e503bb19d83f3cd9a0ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 9 Sep 2026 22:39:39 +0200 Subject: [PATCH 1/6] fix(test): stop the score suites from talking to a real database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three score engines' unit tests hand `persist*Score` a fake Prisma, but `upsertScoreRow` routes the written row through `afterMeasurementMutation`, which reads the `@/lib/db` singleton instead of the injected client. Both legs of that tail therefore opened a socket to a Postgres the unit job does not run: on CI the attempt failed with ECONNREFUSED a few hundred milliseconds later and logged on the way out, after the test that started it had ended. A console write that lands while the worker is closing its RPC channel fails the whole run with `EnvironmentTeardownError: Closing rpc while "onUserConsoleLog" was pending` — four of nine unit-suite failures in thirty days, every one of them with all tests green, blamed on whichever score file the worker happened to be tearing down. Run 31961548185 carries the full stack: recomputeBucketsForMeasurement <- afterMeasurementMutation <- upsertScoreRow <- persistStressScore <- stress-score.test.ts:105. It stayed invisible on a maintainer's machine because a local Postgres answers on 5432: the same call succeeded quietly, and the suite was writing rollup rows for its fixture users into a real database. The four files that drive `upsertScoreRow` stub the tail. That removes the doomed I/O rather than the log line, and the call they were making by accident is now asserted in score-row.test.ts instead: the v1.37.19 wiring is pinned by an expectation rather than by a stack trace. Verified by mutation — dropping the `afterMeasurementMutation` call from score-row.ts fails the new assertion. The settle guard's sweep is widened while here. Its eight-line handler window and five-line registration preamble were both measured against the sites that existed when it was written and both cut through real code: the console write in mood-rollups.ts sits nine lines into its handler, so the one genuinely unregistered detached promise in the tree was never seen, and the registration in fire-and-forget.ts sits below its handler, so a correct site read as a violation. Registration is now looked for wherever it wraps the statement, by paren depth rather than by line count, and mood-rollups.ts registers its WEEK/MONTH/YEAR enqueue. Verified by mutation: unwrapping that call fails the guard. Gate: the four files, twenty consecutive runs, 20/20 green (53 tests each). Test time across the three engine files falls from 708 ms to 20 ms — the round-trips that are no longer made. --- .../background-task-settle-guard.test.ts | 62 +++++++++++++++++-- .../insights/__tests__/recovery-score.test.ts | 12 ++++ src/lib/insights/__tests__/score-row.test.ts | 46 ++++++++++++++ .../insights/__tests__/strain-score.test.ts | 12 ++++ .../insights/__tests__/stress-score.test.ts | 12 ++++ src/lib/rollups/mood-rollups.ts | 42 +++++++------ 6 files changed, 162 insertions(+), 24 deletions(-) diff --git a/src/__tests__/background-task-settle-guard.test.ts b/src/__tests__/background-task-settle-guard.test.ts index 9ce3fa914..322c7dd6f 100644 --- a/src/__tests__/background-task-settle-guard.test.ts +++ b/src/__tests__/background-task-settle-guard.test.ts @@ -19,6 +19,18 @@ * `.catch()` / `.then()` whose handler writes to the console — the shape that * actually produced the failure. A background path that logs some other way, * or that logs from a callee rather than from the handler, is invisible to it. + * + * It was narrower than that until the window below was widened. An + * eight-line handler window and a five-line registration preamble were both + * measured against the sites that existed when the guard was written, and + * both cut through real code: the console write in `mood-rollups.ts` sits + * nine lines into its handler, so the sweep never saw the one detached + * `Promise.all` in the tree that was genuinely unregistered, and the + * registration in `fire-and-forget.ts` sits BELOW its handler rather than + * above it, so a site that is correct read as a violation. The window now + * spans a plausible handler body and registration counts wherever it wraps + * the statement — above the handle or on the line that files the settled + * result. */ import { describe, it, expect } from "vitest"; import { readFileSync } from "node:fs"; @@ -57,6 +69,37 @@ function sourceFiles(): string[] { .filter((rel) => !rel.endsWith(".test.ts") && !rel.endsWith(".test.tsx")); } +/** + * How far past the `.catch(` / `.then(` line a handler's console write still + * counts as this handler's. Twenty lines covers every handler body in the + * tree with room to spare; eight did not. + */ +const HANDLER_WINDOW = 20; + +const registers = (text: string) => /trackBackgroundTask\(/.test(text); + +/** + * The source lines that open a call still unclosed at the start of `line` — + * the chain of enclosing calls the handler sits inside, innermost last. + * + * Paren counting is a coarse reading of TypeScript: a `(` inside a string or a + * comment counts like any other. That costs nothing here, because the question + * asked of the result is only whether one of those lines names + * `trackBackgroundTask`, and a stray paren can at worst widen the chain by a + * line that does not. + */ +function enclosingOpenCallLines(source: string, line: number): string[] { + const lines = source.split("\n"); + const open: number[] = []; + for (let i = 0; i < line - 1; i += 1) { + for (const ch of lines[i]) { + if (ch === "(") open.push(i); + else if (ch === ")") open.pop(); + } + } + return open.map((i) => lines[i]); +} + /** Lines starting a `.catch()` / `.then()` whose handler writes to the console. */ function consoleWritingHandlers(): { file: string; line: number }[] { const found: { file: string; line: number }[] = []; @@ -64,7 +107,7 @@ function consoleWritingHandlers(): { file: string; line: number }[] { const lines = readFileSync(join(SRC, rel), "utf8").split("\n"); lines.forEach((line, index) => { if (!/\.(catch|then)\(/.test(line)) return; - const handler = lines.slice(index, index + 8).join("\n"); + const handler = lines.slice(index, index + HANDLER_WINDOW).join("\n"); if (!/console\.(log|warn|error|info|debug|trace)\(/.test(handler)) return; found.push({ file: rel, line: index + 1 }); }); @@ -149,11 +192,18 @@ describe("background-task settle contract", () => { const unregistered = sites.filter(({ file, line }) => { if (UNREGISTERED_BY_DESIGN.has(file)) return false; - const lines = readFileSync(join(SRC, file), "utf8").split("\n"); - // The registration wraps the handle, so it opens on one of the few lines - // above the `.catch(` / `.then(` the sweep matched. - const preamble = lines.slice(Math.max(0, line - 5), line).join("\n"); - return !/trackBackgroundTask\(/.test(preamble); + const source = readFileSync(join(SRC, file), "utf8"); + // Registered either by wrapping the handle — `trackBackgroundTask(` is + // then one of the calls still open where the handler starts, however + // many lines above that is — or by filing the settled handle just after + // the handler closes. Both shapes are in the tree; a fixed window above + // the match saw neither reliably. + if (enclosingOpenCallLines(source, line).some(registers)) return false; + const lines = source.split("\n"); + const trailer = lines + .slice(line, line + HANDLER_WINDOW + 5) + .filter((l) => registers(l)); + return trailer.length === 0; }); expect( diff --git a/src/lib/insights/__tests__/recovery-score.test.ts b/src/lib/insights/__tests__/recovery-score.test.ts index 375edd22d..12237c9aa 100644 --- a/src/lib/insights/__tests__/recovery-score.test.ts +++ b/src/lib/insights/__tests__/recovery-score.test.ts @@ -12,6 +12,18 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import type { Derived } from "@/lib/insights/derived/types"; import type { ReadinessValue } from "@/lib/insights/derived/readiness"; +/** + * The post-mutation tail reads the real `@/lib/db` client, not the fake this + * file injects, so a stored score opened a socket to a Postgres the unit job + * does not run. The attempt failed a few hundred milliseconds later and logged + * on the way out — after the test that started it had ended — which is how a + * green file tore its worker's RPC channel down mid-write. Stubbed here; + * `score-row.test.ts` asserts the wiring this stands for. + */ +vi.mock("@/lib/rollups/after-measurement-mutation", () => ({ + afterMeasurementMutation: vi.fn(async () => {}), +})); + const computeReadinessMock = vi.fn<(...args: unknown[]) => Promise>>(); diff --git a/src/lib/insights/__tests__/score-row.test.ts b/src/lib/insights/__tests__/score-row.test.ts index 63c4fb4f6..b21f2b2c9 100644 --- a/src/lib/insights/__tests__/score-row.test.ts +++ b/src/lib/insights/__tests__/score-row.test.ts @@ -14,6 +14,27 @@ vi.mock("@/lib/logging/context", () => ({ annotate: (...args: unknown[]) => annotateMock(...args), })); +/** + * The post-mutation tail talks to the real database, and this file does not. + * + * `upsertScoreRow` routes every stored score through + * `afterMeasurementMutation`, which reads the `@/lib/db` singleton rather than + * the client the caller injected. A unit test that hands in a fake Prisma + * therefore still opened a socket to a Postgres the unit job does not run: the + * attempt failed with ECONNREFUSED a few hundred milliseconds later and both + * legs logged on the way out, after the test that started them had ended. A + * console write that lands while the worker is closing its RPC channel fails + * the whole run with `EnvironmentTeardownError: Closing rpc while + * "onUserConsoleLog" was pending` — four of nine unit-suite failures in thirty + * days, every one of them with all tests green. Stubbing the tail removes the + * doomed I/O rather than the log line, and the call it made by accident is + * asserted below instead. + */ +vi.mock("@/lib/rollups/after-measurement-mutation", () => ({ + afterMeasurementMutation: vi.fn(async () => {}), +})); + +import { afterMeasurementMutation } from "@/lib/rollups/after-measurement-mutation"; import { scoreDayKey, scoreMeasuredAt, @@ -92,6 +113,31 @@ describe("upsertScoreRow", () => { measuredAt: arg.create.measuredAt, }); }); + + it("routes the written row through the shared post-mutation tail", async () => { + // v1.37.19 wiring: a score row is a Measurement like any other, so the + // (type, day) rollup bucket and the cached status assessment converge on + // the tick that wrote it. Pinned here rather than left to a live database + // call that no unit job can serve. + const prisma = { + measurement: { upsert: vi.fn().mockResolvedValue({}) }, + } as unknown as Parameters[0]; + + await upsertScoreRow(prisma, { + userId: "u1", + type: "STRAIN_SCORE", + externalIdPrefix: "strain:", + score: 42, + now: new Date("2026-06-02T03:00:00Z"), + }); + + expect(afterMeasurementMutation).toHaveBeenCalledWith("u1", [ + { + type: "STRAIN_SCORE", + measuredAt: new Date("2026-06-01T12:00:00.000Z"), + }, + ]); + }); }); describe("runScoreBatch", () => { diff --git a/src/lib/insights/__tests__/strain-score.test.ts b/src/lib/insights/__tests__/strain-score.test.ts index 1e47ca9f2..6ac225af0 100644 --- a/src/lib/insights/__tests__/strain-score.test.ts +++ b/src/lib/insights/__tests__/strain-score.test.ts @@ -10,6 +10,18 @@ */ import { describe, it, expect, vi, beforeEach } from "vitest"; +/** + * The post-mutation tail reads the real `@/lib/db` client, not the fake this + * file injects, so a stored score opened a socket to a Postgres the unit job + * does not run. The attempt failed a few hundred milliseconds later and logged + * on the way out — after the test that started it had ended — which is how a + * green file tore its worker's RPC channel down mid-write. Stubbed here; + * `score-row.test.ts` asserts the wiring this stands for. + */ +vi.mock("@/lib/rollups/after-measurement-mutation", () => ({ + afterMeasurementMutation: vi.fn(async () => {}), +})); + import { strainDayKey, strainExternalId, diff --git a/src/lib/insights/__tests__/stress-score.test.ts b/src/lib/insights/__tests__/stress-score.test.ts index 1427c4723..b5f5f5391 100644 --- a/src/lib/insights/__tests__/stress-score.test.ts +++ b/src/lib/insights/__tests__/stress-score.test.ts @@ -10,6 +10,18 @@ */ import { describe, it, expect, vi, beforeEach } from "vitest"; +/** + * The post-mutation tail reads the real `@/lib/db` client, not the fake this + * file injects, so a stored score opened a socket to a Postgres the unit job + * does not run. The attempt failed a few hundred milliseconds later and logged + * on the way out — after the test that started it had ended — which is how a + * green file tore its worker's RPC channel down mid-write. Stubbed here; + * `score-row.test.ts` asserts the wiring this stands for. + */ +vi.mock("@/lib/rollups/after-measurement-mutation", () => ({ + afterMeasurementMutation: vi.fn(async () => {}), +})); + const computeVitalsBaselineMock = vi.fn(); vi.mock("@/lib/insights/derived/baseline", () => ({ computeVitalsBaseline: (...args: unknown[]) => diff --git a/src/lib/rollups/mood-rollups.ts b/src/lib/rollups/mood-rollups.ts index 78596eea9..9f9986bd0 100644 --- a/src/lib/rollups/mood-rollups.ts +++ b/src/lib/rollups/mood-rollups.ts @@ -48,6 +48,7 @@ import { prisma } from "@/lib/db"; import { getGlobalBoss } from "@/lib/jobs/boss-instance"; import { annotate } from "@/lib/logging/context"; +import { trackBackgroundTask } from "@/lib/logging/background-tasks"; import { startOfUtcDay } from "@/lib/tz/start-of-utc-day"; import type { Prisma, @@ -307,25 +308,30 @@ export async function recomputeMoodBucketsForEntry( // off the pg-boss critical path. Errors stay caught inside // `enqueueMoodRollupRecompute` so the unhandled-rejection logger // captures any pg-boss failure. - void Promise.all( - ASYNC_GRANULARITIES.map((granularity) => { - const { from, to } = bucketSpan(dateLabel, granularity); - return enqueueMoodRollupRecompute({ - userId, - granularity, - from, - to, - }).catch((err: unknown) => { - const message = err instanceof Error ? err.message : String(err); - annotate({ - meta: { - mood_rollup_enqueue_failed: true, - mood_rollup_enqueue_error: message, - }, + // Registered so the test suite can await it: the error below is a console + // write on a detached handle, and one that lands while a vitest worker is + // closing its RPC channel fails the whole run. No-op outside test. + trackBackgroundTask( + Promise.all( + ASYNC_GRANULARITIES.map((granularity) => { + const { from, to } = bucketSpan(dateLabel, granularity); + return enqueueMoodRollupRecompute({ + userId, + granularity, + from, + to, + }).catch((err: unknown) => { + const message = err instanceof Error ? err.message : String(err); + annotate({ + meta: { + mood_rollup_enqueue_failed: true, + mood_rollup_enqueue_error: message, + }, + }); + console.error("[mood-rollups] async enqueue failed:", message); }); - console.error("[mood-rollups] async enqueue failed:", message); - }); - }), + }), + ), ); } From d48bbff01dbdd20b69cb95847ee79ff6d70d9eb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 9 Sep 2026 22:42:54 +0200 Subject: [PATCH 2/6] ci: take the dependency audit off pull requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pnpm audit --prod` answers a question about the published advisory database, not about the diff. The same lockfile passes on Monday and fails on Tuesday because someone else published, and the branch that turns red changed nothing: seventeen of the forty-one Security & Quality failures in the last thirty days were advisory publications on branches that touched no dependency (GHSA-jqff-g426-hqxp, GHSA-fph4-wmhf-6fwf, GHSA-f65p-4m7j-42xc and GHSA-5jgf-p345-68v8 in one run alone). A gate that fails for a reason the author cannot act on is a gate people re-run instead of read. It keeps running on every push to `main` and once a day on a new schedule trigger, which is the cadence the question actually has. The other three jobs sit out the scheduled run. Nothing about what ships is loosened: the container scan still builds and scans the image on every push to `main`, and the daily Trivy workflow still scans the lockfile with dev dependencies included and the published image. Gate — the trigger set, read back from the parsed workflow: triggers: push, pull_request, schedule quality | if: github.event_name != 'schedule' dependency-audit | if: github.event_name != 'pull_request' secret-scan | if: github.event_name != 'schedule' docker-security | if: github.event_name == 'push' && github.ref == 'refs/heads/main' --- .github/workflows/security.yml | 19 +++++++++++++++++++ .github/workflows/trivy.yml | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 774f04be7..554e4c40e 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -5,6 +5,11 @@ on: branches: [main] pull_request: branches: [main] + schedule: + # Daily, 05:47 UTC. Carries the dependency audit only — every other job + # here answers "is this change safe" and has nothing to say about a tree + # nobody touched. See the note on `dependency-audit`. + - cron: "47 5 * * *" permissions: contents: read @@ -13,6 +18,7 @@ jobs: quality: name: Lint, Typecheck & Test runs-on: ubuntu-latest + if: github.event_name != 'schedule' steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -81,9 +87,21 @@ jobs: fi echo "::notice::OpenAPI spec in sync with Zod source schemas." + # Not on pull requests. `pnpm audit` answers a question about the published + # advisory database, not about the diff: the same lockfile passes on Monday + # and fails on Tuesday because someone else published, and the branch that + # goes red changed nothing. Seventeen of the forty-one failures this workflow + # produced in thirty days were exactly that, and a gate that fails for + # reasons the author cannot act on is a gate people learn to re-run rather + # than read. It runs on every push to `main` and once a day on the schedule + # above, which is the cadence the question actually has. What ships is still + # gated: `docker-security` scans the built image on every push to `main`, and + # `trivy.yml` scans the lockfile (dev dependencies included) and the + # published image daily. dependency-audit: name: Dependency Audit runs-on: ubuntu-latest + if: github.event_name != 'pull_request' steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -109,6 +127,7 @@ jobs: secret-scan: name: Secret Scanning runs-on: ubuntu-latest + if: github.event_name != 'schedule' steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/.github/workflows/trivy.yml b/.github/workflows/trivy.yml index dcc54336d..0bec8cd01 100644 --- a/.github/workflows/trivy.yml +++ b/.github/workflows/trivy.yml @@ -4,7 +4,7 @@ name: Trivy Scheduled Scan # # What already exists and is NOT repeated here: # - `security.yml` → `dependency-audit` runs `pnpm audit --prod` on every -# push and pull request, blocking on HIGH/CRITICAL. +# push to `main` and once a day, blocking on HIGH/CRITICAL. # - `security.yml` → `docker-security` builds the image on a push to `main` # and runs `trivy image` against it, blocking on HIGH/CRITICAL. # From 626a95ff331156c1d8eea1f74fc4ca80e4ac4553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 9 Sep 2026 22:47:31 +0200 Subject: [PATCH 3/6] ci: give every job that runs the unit suite a browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hero-geometry check stopped skipping under CI in v1.38 and started failing instead, which was the point: it had caught its own launch failure and passed for months, measuring nothing. The install step went into the quality job of `security.yml` and nowhere else, and a second job runs the same suite — the auto-merge job for dependency bumps. Every Dependabot pull request has failed since on `browserType.launch: Executable doesn't exist`, four of them on 2026-09-06 alone (run 34043840314), on an error that says nothing about the bump. One workflow was fixed; the class was not. That job installs the same headless shell now, off the same cache key. The error the check throws names both jobs rather than sending a reader to the workflow that was already correct, and a guard sweeps the workflow directory so a third job cannot run the suite with no browser anywhere in it. Its limit is written down: the sweep is file-scoped, so a workflow with two jobs where only one installs still satisfies it. Gates: CI=1, browser path hidden exit=1, "This check is the gate in CI and it has no browser to measure with" CI=1, browser present exit=0, 4 tests passed (it measures) install step deleted from the guard fails: ["dependabot-auto-merge.yml"] auto-merge workflow --- .github/workflows/dependabot-auto-merge.yml | 17 ++++++ .../unit-suite-browser-install-guard.test.ts | 59 +++++++++++++++++++ .../health-score-card-geometry.test.tsx | 21 ++++--- 3 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 src/__tests__/unit-suite-browser-install-guard.test.ts diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 7995cc21f..ef1038e90 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -31,6 +31,23 @@ jobs: - run: pnpm db:generate + # The hero-geometry check in the unit suite renders the real panel in a + # real browser and measures the boxes, and it refuses to skip when CI is + # set — a check that skips measures nothing. This job runs the same suite + # as the quality job in `security.yml` and needs the same browser: without + # it every Dependabot pull request went red on a launch failure that said + # nothing about the bump (four of them on 2026-09-06 alone). Headless + # shell only, matching that job. + - name: Cache Playwright headless shell + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ~/.cache/ms-playwright + key: playwright-shell-${{ runner.os }}-${{ hashFiles('pnpm-lock.yaml') }} + restore-keys: | + playwright-shell-${{ runner.os }}- + + - run: pnpm exec playwright install --only-shell chromium + - run: pnpm test # Auto-merge is gated on the Dependabot-declared update type: diff --git a/src/__tests__/unit-suite-browser-install-guard.test.ts b/src/__tests__/unit-suite-browser-install-guard.test.ts new file mode 100644 index 000000000..aea369329 --- /dev/null +++ b/src/__tests__/unit-suite-browser-install-guard.test.ts @@ -0,0 +1,59 @@ +/** + * Every job that runs the unit suite installs the browser the suite needs. + * + * `health-score-card-geometry.test.tsx` renders the real hero band in a real + * Chromium and measures the boxes. It used to catch its own launch failure and + * pass, so for months it measured nothing in CI; since v1.38 it throws when + * `CI` is set instead, which is right — and which turns a job that runs + * `pnpm test` without a browser from silently useless into loudly red. + * + * That is exactly what happened. The install step was added to the quality job + * in `security.yml` and nowhere else, and `dependabot-auto-merge.yml` runs the + * same suite: every dependency bump failed on a launch error that said nothing + * about the bump. One workflow was fixed, one class was not. + * + * The sweep is textual and file-scoped, which is its limit: a workflow with two + * jobs, only one of which installs the browser, satisfies it. It catches the + * thing that actually went wrong — a whole workflow running the suite with no + * browser anywhere in it. + */ +import { describe, it, expect } from "vitest"; +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; + +const WORKFLOWS = join(process.cwd(), ".github", "workflows"); + +/** `run: pnpm test` — the whole unit suite, geometry check included. */ +const RUNS_UNIT_SUITE = /^\s*(?:-\s*)?run:\s*pnpm test\s*$/m; +const INSTALLS_BROWSER = /playwright install --only-shell chromium/; + +describe("the browser the unit suite measures with", () => { + it("is installed by every workflow that runs the suite", () => { + const files = readdirSync(WORKFLOWS).filter((f) => f.endsWith(".yml")); + expect( + files.length, + "no workflow files found — the walk is broken, not the tree clean", + ).toBeGreaterThanOrEqual(5); + + const runners = files.filter((f) => + RUNS_UNIT_SUITE.test(readFileSync(join(WORKFLOWS, f), "utf8")), + ); + // An empty match set would agree with an empty violation list. It is two + // workflows today; pinned at one so removing a job is not a guard failure, + // but a matcher that stops matching is. + expect( + runners.length, + "no workflow runs `pnpm test` — the matcher stopped matching", + ).toBeGreaterThanOrEqual(1); + + const missing = runners.filter( + (f) => !INSTALLS_BROWSER.test(readFileSync(join(WORKFLOWS, f), "utf8")), + ); + expect( + missing, + "a workflow runs the unit suite without installing the headless " + + "Chromium the hero-geometry check measures with — that check refuses " + + "to skip under CI, so the job fails on a launch error instead", + ).toEqual([]); + }); +}); diff --git a/src/components/insights/__tests__/health-score-card-geometry.test.tsx b/src/components/insights/__tests__/health-score-card-geometry.test.tsx index 5e2b0711d..448e53764 100644 --- a/src/components/insights/__tests__/health-score-card-geometry.test.tsx +++ b/src/components/insights/__tests__/health-score-card-geometry.test.tsx @@ -223,16 +223,23 @@ beforeAll(async () => { browser = await chromium.launch(); } catch (error) { const reason = `no Chromium build available (${String(error).slice(0, 200)})`; - // In CI the browser is installed by the quality job, so a launch failure - // is the job being wrong, not the machine being bare. Throwing from the - // hook fails every test in this suite: a geometry check that quietly - // skips is the same thing as no geometry check, and this repo shipped - // exactly that for months because the skip only ever reached a console - // line nobody reads. + // In CI the browser is installed by the job that runs the suite, so a + // launch failure is the job being wrong, not the machine being bare. + // Throwing from the hook fails every test in this suite: a geometry check + // that quietly skips is the same thing as no geometry check, and this repo + // shipped exactly that for months because the skip only ever reached a + // console line nobody reads. + // + // TWO jobs run `pnpm test`, and naming only one of them here sent a reader + // to a workflow that was already correct: the quality job in + // `security.yml` and the auto-merge job in `dependabot-auto-merge.yml`. + // The second had no install step for three days after this check started + // failing instead of skipping, and turned every dependency bump red. if (process.env.CI) { throw new Error( `[hero geometry] ${reason}. This check is the gate in CI and it has no browser to measure with. ` + - `Restore the "pnpm exec playwright install --only-shell chromium" step in the quality job of .github/workflows/security.yml.`, + `Add the "pnpm exec playwright install --only-shell chromium" step to whichever job ran this suite — ` + + `the quality job of .github/workflows/security.yml, or the auto-merge job of .github/workflows/dependabot-auto-merge.yml.`, ); } skipReason = reason; From 69aac232b4b297685f6139638f73ded4f37ae055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 9 Sep 2026 22:48:16 +0200 Subject: [PATCH 4/6] build: budget the routes on measured drift, not on a ceiling they touch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four watched routes sat within a kilobyte of their ceiling — 444 of 445, 459 of 460, 459 of 460 — because the numbers had been re-stated for four releases and never re-based. A budget a route is already touching does not measure the route; it measures whichever dependency bump landed that week. e2e run 34043772954 failed on `/insights/page: 445 KB gz exceeds the 445 KB budget` with nothing in the diff that touched the bundle, and it was one of at least four e2e failures that were the budget gate rather than a test. The file states the measured value per route now, plus one drift allowance over it, and a route fails on the delta. Measured on a local build at d48bbff01: / 447, /insights 434, /measurements 433, /insights/mood 447, allowance 20 KB, so the effective ceilings are 467 / 454 / 453 / 467 — a raise for all four. The allowance covers the runner delta and ordinary week-to-week drift; the step change the gate exists for, a statically imported catalog or a duplicated chart library, is several times larger and still fails. Every run prints the delta, so a route that has crept most of the way through its allowance is legible in the log long before it is red. The reason and the re-base instructions are written into the file's running note, as with every raise before it. A budget file with no baselines used to gate nothing quietly, since the loop had nothing to walk; it now refuses. Gates, against the current build: real baselines passed; /page +0.3, /insights +0.3, /measurements +0.2, /insights/mood +0.3 KB /page baseline 425 (cap 445), FAILED: "/page: 447 KB gz is 21.6 KB over the /insights 413 (cap 433) 425 KB baseline, past the 20 KB drift allowance (cap 445 KB)" and the same for /insights/page, exit 1 routeBaselineKbGz removed exit 2, "states no routeBaselineKbGz entries, so no route is gated" --- bundle-budget.json | 13 ++++---- scripts/check-bundle-budget.mjs | 59 +++++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/bundle-budget.json b/bundle-budget.json index 868b80aa6..d0777b781 100644 --- a/bundle-budget.json +++ b/bundle-budget.json @@ -1,11 +1,12 @@ { - "$comment": "Client-bundle budgets, enforced by scripts/check-bundle-budget.mjs (--check) after `next build` in CI. Values are KB gzip and include every emitted lazy route and locale chunk, while route budgets include the shared rootMainFiles baseline. Re-measured at v1.37.2: / 455, /insights 443, /measurements 441, /insights/mood 453, total 3328, baseline 130, lazy catalogs 849. Every per-route ceiling is unchanged. The /insights overview crept a kilobyte over its 445 ceiling when the ECG teaser began importing its overview cap from the classification module, dragging that module's label map and helpers into the eager page; the cap now lives in its own value-only module, and the Health Score's single provenance entry is split out of the full metric-provenance map, so the overview imports the bare number and citation it renders rather than every metric's label map behind them. Both are back under 445 with room to spare. The 42 KB aggregate rise from 3286 (v1.35.0) is all locale-catalog strings: v1.36 and v1.37 added the encounter, mood-context, mood-prognosis, practitioner, workout-batch and account-sharing surfaces, and their copy lands across the six locale catalogs the whole-build sum counts independently (804 to 849 KB). The non-catalog client chunks are flat against v1.35.0, so nothing a person downloads on a given route grew beyond its own ceiling. The aggregate cap therefore moves from 3310 to 3352 (24 KB headroom, matching the previous margin); per-route ceilings, the static-catalog guard and the one-Recharts-chunk guard remain unchanged. v1.37.4's honest-error-state charge then added recoverable read-failure handling to thirteen surfaces (the shared QueryErrorCard and QueryErrorRow primitives plus an isError branch per surface) and rewrote the MCP scope note and the managed-record error copy across the six locales; the measured total rose to 3404 while every per-route ceiling stayed met (/page 457, /insights 445, /measurements 443, /insights/mood 455). The aggregate cap moves from 3352 to 3428, keeping the same margin; nothing a person downloads on a given route grew beyond its ceiling. v1.37.15's async-truth charge then added the refresh settling state with its two-locale-key copy, the index-outcome plain-words line (seven keys across the six catalogs) and the reindex-count wiring; the measured total rose to 3429 while every per-route ceiling stayed met (/page 456, /insights 445, /measurements 442, /insights/mood 455). The aggregate cap moves from 3428 to 3452, keeping the same margin; nothing a person downloads on a given route grew beyond its ceiling. v1.37.27 found /insights sitting a few hundred bytes under 445 on main and a few hundred over it on the release branch \u2014 the page had been at its ceiling for three releases and the release only had to breathe on it. The weight it was carrying was not its own: the authenticated shell imported one destination predicate from the `@/lib/record-settings` barrel, and the barrel also re-exports the managed settings-patch schemas, so Zod, the module registry and three validation modules rode into the eager bundle of every authenticated route for a function that reads a URL slug. The shell imports the `classification` leaf now. Measured across all 585 client-rendered routes: none grew by a byte, 100 fell, by 4.7 KB at the median and 8.1 KB at the most; /insights 444 to 437, /measurements 442 to 435, / 456 to 450, /insights/mood 455 to 450. The whole-build sum moved the other way, 3451 to 3476, and that direction is the metric rather than the change: the aggregate counts each emitted chunk once, so a graph that leaves the chunk group every route shares gets counted again wherever it lands \u2014 here inside the deferred settings chunks that actually use it. Bytes on disk rose 25 KB; bytes a reader downloads only ever fell. The aggregate cap moves from 3452 to 3500, the same 24 KB headroom as the four re-measurements before it. Per-route ceilings keep their old values rather than tightening to the new floor: the CI runner measures each route about 1.5 KB heavier than a local arm64 build does, and a ceiling set from a local number would fail on a build that had not regressed. The static-catalog guard and the one-Recharts-chunk guard remain unchanged. Raised to 3700 at v1.37.32 for the seventh interface language (Korean): the lazy message catalogs went 6->7 chunks (1026 KB gz measured), pushing the aggregate to 3631 against the old 3500. The raise is entirely the new catalog; no route budget moved, and the four eager route ceilings are unchanged and still passing. Most of the aggregate is the dynamic locale-switch and fallback path that a first paint never loads (see issue #253), so this ceiling guards bytes almost no visitor requests; 3700 leaves ~69 KB for ordinary drift until that double-count is cleaned up.", - "routesKbGz": { - "/page": 460, - "/insights/page": 445, - "/measurements/page": 445, - "/insights/mood/page": 460 + "$comment": "Client-bundle budgets, enforced by scripts/check-bundle-budget.mjs (--check) after `next build` in CI. Values are KB gzip and include every emitted lazy route and locale chunk, while route budgets include the shared rootMainFiles baseline. Re-measured at v1.37.2: / 455, /insights 443, /measurements 441, /insights/mood 453, total 3328, baseline 130, lazy catalogs 849. Every per-route ceiling is unchanged. The /insights overview crept a kilobyte over its 445 ceiling when the ECG teaser began importing its overview cap from the classification module, dragging that module's label map and helpers into the eager page; the cap now lives in its own value-only module, and the Health Score's single provenance entry is split out of the full metric-provenance map, so the overview imports the bare number and citation it renders rather than every metric's label map behind them. Both are back under 445 with room to spare. The 42 KB aggregate rise from 3286 (v1.35.0) is all locale-catalog strings: v1.36 and v1.37 added the encounter, mood-context, mood-prognosis, practitioner, workout-batch and account-sharing surfaces, and their copy lands across the six locale catalogs the whole-build sum counts independently (804 to 849 KB). The non-catalog client chunks are flat against v1.35.0, so nothing a person downloads on a given route grew beyond its own ceiling. The aggregate cap therefore moves from 3310 to 3352 (24 KB headroom, matching the previous margin); per-route ceilings, the static-catalog guard and the one-Recharts-chunk guard remain unchanged. v1.37.4's honest-error-state charge then added recoverable read-failure handling to thirteen surfaces (the shared QueryErrorCard and QueryErrorRow primitives plus an isError branch per surface) and rewrote the MCP scope note and the managed-record error copy across the six locales; the measured total rose to 3404 while every per-route ceiling stayed met (/page 457, /insights 445, /measurements 443, /insights/mood 455). The aggregate cap moves from 3352 to 3428, keeping the same margin; nothing a person downloads on a given route grew beyond its ceiling. v1.37.15's async-truth charge then added the refresh settling state with its two-locale-key copy, the index-outcome plain-words line (seven keys across the six catalogs) and the reindex-count wiring; the measured total rose to 3429 while every per-route ceiling stayed met (/page 456, /insights 445, /measurements 442, /insights/mood 455). The aggregate cap moves from 3428 to 3452, keeping the same margin; nothing a person downloads on a given route grew beyond its ceiling. v1.37.27 found /insights sitting a few hundred bytes under 445 on main and a few hundred over it on the release branch — the page had been at its ceiling for three releases and the release only had to breathe on it. The weight it was carrying was not its own: the authenticated shell imported one destination predicate from the `@/lib/record-settings` barrel, and the barrel also re-exports the managed settings-patch schemas, so Zod, the module registry and three validation modules rode into the eager bundle of every authenticated route for a function that reads a URL slug. The shell imports the `classification` leaf now. Measured across all 585 client-rendered routes: none grew by a byte, 100 fell, by 4.7 KB at the median and 8.1 KB at the most; /insights 444 to 437, /measurements 442 to 435, / 456 to 450, /insights/mood 455 to 450. The whole-build sum moved the other way, 3451 to 3476, and that direction is the metric rather than the change: the aggregate counts each emitted chunk once, so a graph that leaves the chunk group every route shares gets counted again wherever it lands — here inside the deferred settings chunks that actually use it. Bytes on disk rose 25 KB; bytes a reader downloads only ever fell. The aggregate cap moves from 3452 to 3500, the same 24 KB headroom as the four re-measurements before it. Per-route ceilings keep their old values rather than tightening to the new floor: the CI runner measures each route about 1.5 KB heavier than a local arm64 build does, and a ceiling set from a local number would fail on a build that had not regressed. The static-catalog guard and the one-Recharts-chunk guard remain unchanged. Raised to 3700 at v1.37.32 for the seventh interface language (Korean): the lazy message catalogs went 6->7 chunks (1026 KB gz measured), pushing the aggregate to 3631 against the old 3500. The raise is entirely the new catalog; no route budget moved, and the four eager route ceilings are unchanged and still passing. Most of the aggregate is the dynamic locale-switch and fallback path that a first paint never loads (see issue #253), so this ceiling guards bytes almost no visitor requests; 3700 leaves ~69 KB for ordinary drift until that double-count is cleaned up. v1.38.14 re-measures the four eager routes and changes what the number means. The ceilings had been re-stated but never re-based since v1.37.4, so /insights and /measurements sat within a kilobyte of theirs and / and /insights/mood within one of theirs; e2e run 34043772954 failed on `/insights/page: 445 KB gz exceeds the 445 KB budget` with the other two at 459 of 460, and none of it was a regression anyone had made. A ceiling a route is already touching does not measure the route, it measures whichever dependency bump landed that week. The file states the measured value per route now, plus one drift allowance over it: measured on a local arm64 build at d48bbff01 as / 447, /insights 434, /measurements 433, /insights/mood 447, with a 20 KB allowance, so the effective ceilings are 467 / 454 / 453 / 467 — a raise for all four. The allowance covers the runner delta (the CI runner measures each route a little heavier than a local arm64 build) and ordinary week-to-week drift, while the step change the gate exists for — a statically imported message catalog, a duplicated chart library — is several times larger and still fails. Every run now prints the delta against the baseline, so a route that has crept 15 KB is legible in the log long before it is red; the flat ceiling could only ever say pass or fail. Re-base by re-measuring on a build with nothing else in it and writing the new numbers here with the reason, exactly as a raised ceiling was written before.", + "routeBaselineKbGz": { + "/page": 447, + "/insights/page": 434, + "/measurements/page": 433, + "/insights/mood/page": 447 }, + "routeDriftAllowanceKbGz": 20, "totalClientKbGz": 3700, "maxRechartsChunks": 1 } diff --git a/scripts/check-bundle-budget.mjs b/scripts/check-bundle-budget.mjs index cd2518981..f53c6f44d 100644 --- a/scripts/check-bundle-budget.mjs +++ b/scripts/check-bundle-budget.mjs @@ -23,11 +23,20 @@ * pnpm bundle-report # print the report table * pnpm bundle-report --check # enforce bundle-budget.json (CI gate) * - * Budgets live in `bundle-budget.json` (repo root). Numbers are KB gzip - * with deliberate headroom over the measured value — the gate exists to - * catch step-change regressions (a ~100 KB catalog, a ~90 KB duplicate - * library), not day-to-day noise. Raise a budget consciously, in the same - * PR that pays the cost, with the reason in the PR body. + * Budgets live in `bundle-budget.json` (repo root). Numbers are KB gzip. + * + * Per-route the file states a MEASURED baseline and one drift allowance + * over it, and a route fails on the delta rather than on a ceiling it is + * already touching. Flat ceilings had been re-stated but never re-based, so + * three routes ended up sitting within a kilobyte of theirs and any + * unrelated dependency bump failed the e2e job on a regression nobody had + * made. The delta is printed on every run, so a route that has crept most of + * the way through its allowance is readable in the log before it is red. + * + * The gate still exists to catch step changes (a ~100 KB statically imported + * catalog, a ~90 KB duplicate library), which are several times the + * allowance. Re-base consciously, in the same PR that pays the cost, with + * the reason written into `$comment`. */ import { readFileSync, readdirSync, existsSync } from "node:fs"; @@ -166,14 +175,28 @@ console.log( console.log(`recharts-fingerprint chunks: ${rechartsChunks}`); console.log(""); -const watched = budget ? Object.keys(budget.routesKbGz ?? {}) : []; +const baselines = budget?.routeBaselineKbGz ?? {}; +const allowanceKb = budget?.routeDriftAllowanceKbGz ?? 0; +/** The ceiling a route's measured baseline plus the drift allowance buys it. */ +const capFor = (route) => + baselines[route] == null ? null : baselines[route] + allowanceKb; + +const watched = Object.keys(baselines); const rows = [...routeTotals.entries()] .filter(([route]) => !watched.length || watched.includes(route)) .sort((a, b) => b[1].gz - a[1].gz); for (const [route, { gz }] of rows.slice(0, watched.length || 15)) { - const cap = budget?.routesKbGz?.[route]; + const baseline = baselines[route]; + if (baseline == null) { + console.log(`${fmt(gz).padStart(8)} gz ${route}`); + continue; + } + const delta = kb(gz) - baseline; + const sign = delta >= 0 ? "+" : "−"; console.log( - `${fmt(gz).padStart(8)} gz ${route}${cap ? ` (budget ${cap} KB)` : ""}`, + `${fmt(gz).padStart(8)} gz ${route} (baseline ${baseline} KB, ` + + `${sign}${Math.abs(delta).toFixed(1)} KB of ${allowanceKb} KB allowed, ` + + `cap ${capFor(route)} KB)`, ); } @@ -185,16 +208,30 @@ if (!budget) { process.exit(2); } +// A budget file that states no baselines gates nothing, and would do it +// quietly — the loop below would simply have nothing to walk. Say so instead. +if (watched.length === 0) { + console.error( + "check-bundle-budget: bundle-budget.json states no routeBaselineKbGz " + + "entries, so no route is gated. Measure the eager routes and write " + + "them in, with a routeDriftAllowanceKbGz over them.", + ); + process.exit(2); +} + const failures = []; -for (const [route, capKb] of Object.entries(budget.routesKbGz ?? {})) { +for (const [route, baselineKb] of Object.entries(baselines)) { const actual = routeTotals.get(route); if (!actual) { failures.push(`route ${route} not found in the build output`); continue; } - if (kb(actual.gz) > capKb) { + const delta = kb(actual.gz) - baselineKb; + if (delta > allowanceKb) { failures.push( - `${route}: ${fmt(actual.gz)} gz exceeds the ${capKb} KB budget`, + `${route}: ${fmt(actual.gz)} gz is ${delta.toFixed(1)} KB over the ` + + `${baselineKb} KB baseline, past the ${allowanceKb} KB drift ` + + `allowance (cap ${capFor(route)} KB)`, ); } } From 7ca69ec671075783d899f64cdc208b4504ed1dd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 9 Sep 2026 22:55:53 +0200 Subject: [PATCH 5/6] test(integration): stop the rate-limit reset test racing its own window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The window-reset test burned its budget in a 50 ms window: four sequential round-trips to a containerised Postgres had to finish inside a twentieth of a second or the window closed under them, the counter reset, and the fourth call came back allowed. Three failures in thirty days, two of them on `main` — run 33614804032 shows `expected true to be false` at rate-limit.test.ts:56 in a file that took 1278 ms to run. The short window bought nothing: the expiry this test is about is forced by hand a few lines further down, by writing `reset_at` into the past. The burn now runs in a 60 s window, so no round-trip can outlast it, and the test asserts the row's count is 4 after the denial — a window that closed mid-burn would also have produced three allowed calls, and the old assertion could not tell the two apart. Gates: pnpm exec vitest run --config vitest.integration.config.mts \ tests/integration/rate-limit.test.ts three consecutive runs, 2 passed / 2 passed / 2 passed. Mutation: with the burn window back at 1 ms the run fails exactly as CI did — "AssertionError: expected true to be false" on the denial assertion. --- tests/integration/rate-limit.test.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/integration/rate-limit.test.ts b/tests/integration/rate-limit.test.ts index fa0d3b494..3f9d8e7ee 100644 --- a/tests/integration/rate-limit.test.ts +++ b/tests/integration/rate-limit.test.ts @@ -47,14 +47,30 @@ describe("checkRateLimit (real Postgres)", () => { const key = "test:reset:1.2.3.4"; const limit = 3; - // Burn the budget with a short 50ms window. + // Burn the budget in a window wide enough that it cannot close under the + // four calls that fill it. It used to be 50 ms, which asked the four + // round-trips to a containerised Postgres to finish inside a twentieth of + // a second: on a loaded runner the window expired mid-burn, the counter + // reset, and the fourth call came back allowed — three failures in thirty + // days, two of them on `main` (run 33614804032, `expected true to be + // false` on the line below, in a file that took 1278 ms to run). The short + // window bought nothing either way, because the expiry this test is about + // is forced by hand a few lines down rather than waited for. + const windowMs = 60_000; for (let i = 0; i < 3; i++) { - const r = await checkRateLimit(key, limit, 50); + const r = await checkRateLimit(key, limit, windowMs); expect(r.allowed).toBe(true); } - const denied = await checkRateLimit(key, limit, 50); + const denied = await checkRateLimit(key, limit, windowMs); expect(denied.allowed).toBe(false); + // The denial has to be the cap, not a window that closed under the burn — + // a reset would also have produced three allowed calls before it. + const burned = await getPrismaClient().rateLimit.findUnique({ + where: { key }, + }); + expect(burned?.count).toBe(4); + // Manually expire the window: cheaper than sleeping and avoids // flaky timing on slow CI runners. The branch under test compares // `reset_at < NOW()`, so any past timestamp triggers the reset. From de7c89a46228686868ef9cc0885c56d79d3e75aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 00:20:30 +0200 Subject: [PATCH 6/6] ci: keep the dependency audit visible on pull requests without blocking them The audit had moved off the pull_request trigger entirely, which left a window in which a tag could be cut from a merge the audit had not seen. It runs on pull requests again as an advisory job (continue-on-error), stays blocking on the push run for main and on the daily schedule, and a tag is only ever cut from a green push run. --- .github/workflows/security.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 554e4c40e..ba9c50a56 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -101,7 +101,11 @@ jobs: dependency-audit: name: Dependency Audit runs-on: ubuntu-latest - if: github.event_name != 'pull_request' + # Runs on pull requests too, but advisory: an advisory published against a + # dependency the branch did not touch must not block the branch. The push + # run on main and the daily schedule stay blocking, and a tag is only ever + # cut from a main whose push run is green. + continue-on-error: ${{ github.event_name == 'pull_request' }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1