diff --git a/.github/actions/setup-lighthouse-chromium/action.yml b/.github/actions/setup-lighthouse-chromium/action.yml new file mode 100644 index 0000000000..4e131508eb --- /dev/null +++ b/.github/actions/setup-lighthouse-chromium/action.yml @@ -0,0 +1,49 @@ +name: Setup pinned Chromium for Lighthouse +description: > + Install Playwright's managed Chromium and export PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH, so every + Lighthouse job in this repository measures with the same browser build for a given commit. + +# The Lighthouse budget used to drive whatever Chrome ships in the ubuntu-24.04 runner image +# (matching live-web-vitals.yml, which measures a live domain and isn't graded against a +# committed baseline). That ambient browser is NOT pinned per commit — the runner fleet was +# observed serving HeadlessChrome/150 and /151 to jobs minutes apart on 2026-08-07 — so the +# baseline comparator treated the version drift as "evidence incomplete" and failed closed +# (by design: a browser bump is otherwise indistinguishable from a real regression). +# Refreshing the baseline for whichever version showed up bought a few hours before the next +# mismatched runner failed it again (#1690). +# +# Pin Playwright's own managed Chromium instead, the same binary the Production UI / Visual +# baselines jobs already use, cached by lockfile hash so it is identical across every runner +# for a given commit. run-lighthouse-budget.mjs already honours +# CHROME_PATH/PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH. +# +# This lives in a composite action rather than being duplicated per job for one reason: the +# measuring job and the baseline-refresh job MUST resolve the same executable. If they drift, +# the refreshed baseline records a browser other than the one that will grade against it, and +# the gate goes permanently red again — exactly the failure this action exists to end. + +runs: + using: composite + steps: + - name: Restore Chromium browser cache + id: pw-cache + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 + with: + path: ~/.cache/ms-playwright + key: playwright-chromium-${{ runner.os }}-${{ hashFiles('package-lock.json') }} + restore-keys: | + playwright-chromium-${{ runner.os }}- + + - name: Install Chromium + shell: bash + run: | + if [ "${{ steps.pw-cache.outputs.cache-hit }}" = "true" ]; then + npx playwright install-deps chromium + npx playwright install chromium + else + npx playwright install --with-deps chromium + fi + + - name: Pin the Chromium executable path + shell: bash + run: echo "PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=$(node -e "console.log(require('playwright').chromium.executablePath())")" >> "$GITHUB_ENV" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c524f7cbd..77b3e8be4a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -8,6 +8,15 @@ on: types: [opened, synchronize, reopened, ready_for_review] merge_group: workflow_dispatch: + inputs: + refresh_lighthouse_baseline: + description: > + Re-measure the budgeted routes on this runner's PINNED Chromium and upload a refreshed + lighthouse-budget.json as an artifact for a human to review and commit. The baseline is a + browser-specific artefact, so it can only be produced on a runner — never from a developer + machine. Nothing is committed or pushed by this run. + type: boolean + default: false schedule: - cron: "0 18 * * 0" @@ -39,6 +48,7 @@ jobs: static_heavy_changed: ${{ steps.scope.outputs.static_heavy_changed }} coverage_changed: ${{ steps.scope.outputs.coverage_changed }} ui_changed: ${{ steps.scope.outputs.ui_changed }} + perf_changed: ${{ steps.scope.outputs.perf_changed }} advisory_ui_changed: ${{ steps.scope.outputs.advisory_ui_changed }} db_changed: ${{ steps.scope.outputs.db_changed }} container_changed: ${{ steps.scope.outputs.container_changed }} @@ -706,7 +716,62 @@ jobs: lighthouse-budget: name: Lighthouse budget (advisory) needs: changes - if: needs.changes.outputs.ui_changed == 'true' || needs.changes.outputs.build_changed == 'true' + # Runs when the diff can plausibly move LCP/TBT/CLS on the five budgeted routes, + # and when the run is one whose result someone will act on. It used to key off + # `ui_changed || build_changed`, which put every dependabot lockfile bump (#1668) + # and every worker/** ingestion change through a ~7 minute isolated production + # build plus ten Lighthouse runs, with zero render-path relevance. + # + # pull_request – perf scope, non-draft. Drafts churn and this costs ~7 min; + # `types:` already includes ready_for_review, so undrafting + # re-triggers it without a push. + # merge_group – SKIPPED. The job is continue-on-error and is not in + # pr-required, so in the queue it can only add ~7 minutes of + # merge latency and can never change the outcome. If it is + # ever promoted (#118), merge_group MUST come back — + # tests/ci-cache-safety.test.ts enforces that pairing. + # push (main/…) – perf scope, PLUS lockfile_changed. The lockfile arm is the + # backstop for a runtime dependency bump: perf scope excludes + # package.json / package-lock.json because ci-change-scope.mjs + # sees paths, not lockfile contents, so without this arm a + # lockfile-only push would skip Lighthouse on both the PR and + # the subsequent push to main and leave only the weekly schedule. + # schedule – weekly; resolves to the full-run sentinel, which trips perf + # scope (asserted in ci-change-scope.mjs's self-tests). + # workflow_dispatch – always, except a run dispatched to refresh the baseline + # (that belongs to lighthouse-baseline-refresh below). + # + # Labels mirror the codex-review / skip-codex-review convention; an explicit skip + # always wins. `on.pull_request.types` has no `labeled`, so the opt-in label takes + # effect on the next synchronize or re-run — adding `labeled` would re-run all of + # CI on every label change, so the escape hatch is workflow_dispatch instead. + # + # Two expression traps, both load-bearing: + # - `github.event.inputs.X` is a STRING ('true'/'false') and is safely null on + # non-dispatch events; the `inputs` context is a real boolean but only exists + # for workflow_dispatch/workflow_call. Use the github.event.inputs form. + # - `github.event.pull_request` is null on push/schedule/merge_group, so + # `draft != true` is true there. Do NOT write `== false`: that is false on + # every non-PR event and would silently kill the push and schedule arms. + if: > + ( + github.event_name == 'schedule' || + github.event_name == 'workflow_dispatch' || + contains(github.event.pull_request.labels.*.name, 'lighthouse-budget') || + ( + ( + needs.changes.outputs.perf_changed == 'true' || + ( + github.event_name == 'push' && + needs.changes.outputs.lockfile_changed == 'true' + ) + ) && + github.event_name != 'merge_group' && + github.event.pull_request.draft != true + ) + ) && + !contains(github.event.pull_request.labels.*.name, 'skip-lighthouse-budget') && + github.event.inputs.refresh_lighthouse_baseline != 'true' continue-on-error: true runs-on: ubuntu-24.04 timeout-minutes: 45 @@ -719,40 +784,8 @@ jobs: - name: Setup Node and dependencies uses: ./.github/actions/setup-node-cached - # This job used to drive whatever Chrome ships in the ubuntu-24.04 runner image - # (matching live-web-vitals.yml, which measures a live domain and isn't graded - # against a committed baseline). That ambient browser is NOT pinned per commit — - # the runner fleet was observed serving HeadlessChrome/150 and /151 to jobs - # minutes apart on 2026-08-07 — so every grading run this job's baseline - # comparator treats a version drift as "evidence incomplete" and fails closed - # (by design: a browser bump is otherwise indistinguishable from a real - # regression). Refreshing the baseline for whichever version showed up bought a - # few hours before the next mismatched runner failed it again (#1690). - # - # Pin Playwright's own managed Chromium instead, the same binary the Production - # UI / Visual baselines jobs already use, cached by lockfile hash so it is - # identical across every runner for a given commit. run-lighthouse-budget.mjs - # already honours CHROME_PATH/PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH. - - name: Restore Chromium browser cache - id: pw-cache - uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6 - with: - path: ~/.cache/ms-playwright - key: playwright-chromium-${{ runner.os }}-${{ hashFiles('package-lock.json') }} - restore-keys: | - playwright-chromium-${{ runner.os }}- - - - name: Install Chromium - run: | - if [ "${{ steps.pw-cache.outputs.cache-hit }}" = "true" ]; then - npx playwright install-deps chromium - npx playwright install chromium - else - npx playwright install --with-deps chromium - fi - - - name: Pin the Chromium executable path - run: echo "PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH=$(node -e "console.log(require('playwright').chromium.executablePath())")" >> "$GITHUB_ENV" + - name: Setup pinned Chromium + uses: ./.github/actions/setup-lighthouse-chromium - name: Measure routes and grade against the baseline run: npm run verify:lighthouse -- --keep --dir lighthouse @@ -765,6 +798,66 @@ jobs: path: lighthouse/ if-no-files-found: ignore + # Refresh the committed Lighthouse baseline FROM A CI RUNNER. Dispatch-only. + # + # The baseline is a browser-specific artefact: check-lighthouse-budget.mjs fails + # closed when a baseline row's chromeVersion differs from the measuring run's, + # because a browser bump is otherwise indistinguishable from an application + # regression. So the numbers must come from the same pinned Chromium the grading job + # uses, which a developer machine cannot supply (#118: never commit baselines from a + # developer machine). This job produces the file; a human reviews the diff and + # commits it. That division is the point — a workflow that can rewrite a gate's own + # baseline is a gate that can green itself. + # + # Deliberately NOT continue-on-error: a refresh that measured nothing must go red. + lighthouse-baseline-refresh: + name: Refresh Lighthouse baseline (dispatch only) + if: github.event_name == 'workflow_dispatch' && github.event.inputs.refresh_lighthouse_baseline == 'true' + runs-on: ubuntu-24.04 + timeout-minutes: 45 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Setup Node and dependencies + uses: ./.github/actions/setup-node-cached + + - name: Setup pinned Chromium + uses: ./.github/actions/setup-lighthouse-chromium + + # --update refuses on a measurement gap (incompleteBudgetEvidence with + # ignoreBaseline:true still blocks a missing or unusable report) and accepts + # browser drift, which is the reason to refresh. --keep retains the raw reports + # so the artifact carries evidence, not just a conclusion. + - name: Measure and rewrite the baseline + run: npm run verify:lighthouse -- --update --keep --dir lighthouse + + # --update returns before rendering the table, so without this nothing describes + # what was recorded. Re-grading the same reports against the just-written + # baseline yields zero deltas by construction: this RENDERS the recorded numbers + # into the step summary, it does not verify them. + - name: Render the recorded baseline into the run summary + run: npm run check:lighthouse-budget -- --dir lighthouse + + # The acceptance check for this job: exactly one distinct browser, and it is the + # pinned one. More than one line here means the refresh is not usable. + - name: Show the baseline diff and the browser it was measured on + run: | + git --no-pager diff --stat -- lighthouse-budget.json + node -e "const b=require('./lighthouse-budget.json');console.log([...new Set(Object.values(b.baseline??{}).map(r=>r.chromeVersion))].join('\n'))" + + - name: Upload the refreshed baseline + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: lighthouse-baseline-refresh-${{ github.run_id }} + path: | + lighthouse-budget.json + lighthouse/ + if-no-files-found: error + db-reset-verify: name: Migration replay needs: changes diff --git a/docs/process-hardening.md b/docs/process-hardening.md index 1adb19edb6..4f71bdc1cc 100644 --- a/docs/process-hardening.md +++ b/docs/process-hardening.md @@ -301,10 +301,30 @@ Three gates added for the "mature repo" verification pass. Full usage is in - **The performance budget is relative, not absolute.** `lighthouse-budget.json` holds a committed per-route baseline and a per-metric tolerance, following `check:bundle-budget`. Absolute web-vitals thresholds are meaningless against localhost with no network latency. `enforce` starts - `false` with no baseline; incomplete evidence fails regardless of `enforce`. + `false`; incomplete evidence fails regardless of `enforce`. +- **The baseline is browser-specific, and the browser is now pinned (2026-08-08).** Each baseline row + records the `chromeVersion` that produced it, and a mismatch fails closed — a browser bump is + otherwise indistinguishable from an application regression. The ambient runner-image Chrome is not + pinned per commit (the fleet served HeadlessChrome/150 and /151 to jobs minutes apart on + 2026-08-07), so both Lighthouse jobs resolve Playwright's managed Chromium through one shared + composite action, `./.github/actions/setup-lighthouse-chromium`. Refreshing is a `workflow_dispatch` + input (`refresh_lighthouse_baseline`) that uploads a rewritten `lighthouse-budget.json` for a human + to review and commit; it deliberately cannot push, because a workflow that rewrites a gate's own + baseline is a gate that can green itself. +- **The budget runs on perf scope, not the ui/build union (2026-08-08).** `perf_changed` in + `scripts/ci-change-scope.mjs` excludes `worker/**` and container surfaces, dependency manifests and + the lockfile, Playwright/test surfaces, most of `src/app/api/**` (except the initial-load handlers + `/api/setup-status` and `/api/local-project-id`), and `src/app/mockups/**`. `src/proxy.ts` stays in + scope because it runs before every budgeted navigation. The lockfile exclusion is the one + deliberate hole on the PR arm: the classifier sees paths, never the lockfile diff, so it cannot + tell a devDependency bump from a React/Next bump. The push-to-`main` arm re-runs when + `lockfile_changed` is true, and `check:bundle-budget` still covers the same PRs. If a runtime dep + bump ever lands an unnoticed LCP regression despite that, teach the classifier to read the lockfile + diff — do not put the whole lockfile back into perf scope. - **Verification debt remaining.** (1) No pixel baselines are committed, so `visual-baseline` reports - rather than gates until an operator adopts them from the CI artifact. (2) No Lighthouse baseline is - recorded, so the budget warns rather than grades until `--update` runs against a known-good build. + rather than gates until an operator adopts them from the CI artifact. (2) A Lighthouse baseline is + committed, but `enforce` stays `false` until #147 (`/dsm` mobile CLS 0.363) and #117 are resolved — + enforcing today would ratify those breaches. (3) 37 of the 38 unlayered visual classes carry exemptions rather than effect contracts — the debt is enumerated in `STYLE_CONTRACT_EXEMPTIONS`, and ledger #094 stays open until the load-bearing ones have contracts. (4) `scripts/run-lighthouse-budget.mjs` duplicates roughly 50 lines of the diff --git a/docs/scripts-index.md b/docs/scripts-index.md index d24c8c5a17..411ad1216f 100644 --- a/docs/scripts-index.md +++ b/docs/scripts-index.md @@ -1,6 +1,6 @@ # Scripts index -Curated map of `scripts/` (218 files) and the `package.json` script surface (231 entries), +Curated map of `scripts/` (219 files) and the `package.json` script surface (231 entries), grouped by purpose. This is orientation, not an exhaustive per-file listing — the authoritative command list is `package.json`, and `npm run docs:check-scripts` verifies every `npm run ` referenced in docs resolves to a real script. `npm run docs:update` refreshes the exact counts above. diff --git a/docs/testing.md b/docs/testing.md index 6801fb9272..67058524c0 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -133,8 +133,62 @@ following the same shape as `check:bundle-budget`. route counted as a pass is the failure mode `summarise-web-vitals.mjs` documents at length. - CLS is graded on absolute movement; LCP and TBT need to clear both a percentage and an absolute floor, so 12 ms → 16 ms is not reported as a 33% regression. - -Refresh the baseline deliberately after a known-good run: `npm run check:lighthouse-budget -- --update`. +- A cell that produced **no measurement at all** — a non-zero exit, no report file, or a report + carrying only a `runtimeError` such as Lighthouse's own `NO_NAVSTART` (ledger #147 recorded + `/forms` doing this) — is retried **once**, and the retry is always logged to the run summary and + written to `retries.txt` whether or not it recovered. A run that never started measured nothing + about the diff; it is not a pass either, so if the retry also produces nothing the grader still + fails closed. A cell that _did_ measure and produced bad numbers is never retried. + +### Baseline browser pinning, and how to refresh + +The baseline is a **browser-specific** artefact. `check-lighthouse-budget.mjs` fails closed when a +baseline row's `chromeVersion` differs from the measuring run's, because a browser bump is otherwise +indistinguishable from an application regression. Both Lighthouse jobs therefore pin Playwright's +managed Chromium through the shared `./.github/actions/setup-lighthouse-chromium` composite action — +never the ambient runner-image Chrome, which is not pinned per commit (the fleet was observed serving +HeadlessChrome/150 and /151 to jobs minutes apart on 2026-08-07). Drift is reported as one collapsed +instruction rather than one sentence per route, but the verdict is unchanged: incomplete evidence +still fails, independently of `enforce`. + +Because the numbers must come from that pinned browser, refresh the baseline **from a CI runner**, +never a developer machine: + +1. Actions → CI → **Run workflow** → pick the branch → tick **refresh_lighthouse_baseline** → Run. +2. Check the run's diff step prints exactly **one** distinct `chromeVersion`, and that it is the + pinned `HeadlessChrome/`. More than one line means the refresh is not usable. +3. Download the `lighthouse-baseline-refresh-` artifact, review the per-route deltas, and + commit **only** `lighthouse-budget.json`. + +The refresh job is dispatch-only, is not `continue-on-error` (a refresh that measured nothing must go +red), and deliberately cannot push — a workflow that can rewrite a gate's own baseline is a gate that +can green itself. `npm run check:lighthouse-budget -- --update` exists for local experiments; its +output must not be committed. + +### When the budget runs + +Keyed off `perf_changed` (`scripts/ci-change-scope.mjs`), which is deliberately narrower than the +`ui_changed || build_changed` union it replaced — that union put every dependabot lockfile bump and +every `worker/**` ingestion change through a ~7 minute isolated production build plus ten Lighthouse +runs. In scope: `src/**`, `data/**`, `public/**`, `next.config.ts`, `postcss.config.mjs`, +`tsconfig.json`, the Chromium pin composite action, and the budget's own inputs. Excluded: +`worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces +including committed screenshots, most of `src/app/api/**` (except the initial-load handlers +`/api/setup-status` and `/api/local-project-id` that `/` always fetches), `src/app/mockups/**`, and +server/edge runtime entry points other than `src/proxy.ts` (which runs before every budgeted +navigation). An unrecognised path under a listed root stays **in** scope, so a future refactor +over-triggers by one job rather than silently dropping a render surface. + +Event matrix: pull requests on perf scope when not a draft; `merge_group` skipped while the job is +advisory (it could only add merge latency without changing the outcome — promoting it to +`pr-required` must restore `merge_group` in the same edit, which `tests/ci-cache-safety.test.ts` +enforces); `push` to `main`/`release/**` on perf scope **or** when `lockfile_changed` is true (the +lockfile arm is the backstop for a runtime dependency bump the PR arm deliberately excludes from +`perf_changed`); the weekly `schedule`; and `workflow_dispatch`. The `lighthouse-budget` label +forces a run and `skip-lighthouse-budget` opts out, with skip winning. Caveat: +`on.pull_request.types` has no `labeled` (adding it would re-run all of CI on every label change), +so the opt-in label takes effect on the next push or re-run — use `workflow_dispatch` for an +immediate run. This is distinct from `.github/workflows/live-web-vitals.yml`, which measures the deployed origin for ledger #017 and is dispatch-only — by the time it runs, `main` has already auto-deployed. Both pin @@ -153,7 +207,7 @@ UI scope runs a fail-fast `@critical` Chromium job on pull requests, then one re PR body synchronization is skipped unless the checked-out head actually contains `PR_POLICY_BODY.md`. The eval-canary liveness API probe runs once with the daily Ops Digest cadence rather than on every PR. These remove repeated provider-side work without weakening a required result. -Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on UI-or-build scope. Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit. +Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on the narrower perf scope (see "When the budget runs" above — `worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces, most of `src/app/api/**` other than initial-load handlers, and `src/app/mockups/**` are excluded; `src/proxy.ts` stays in). Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit; for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`. ## Contribution checklist (UI changes) diff --git a/scripts/check-lighthouse-budget.mjs b/scripts/check-lighthouse-budget.mjs index bc549bd45a..e175baa691 100644 --- a/scripts/check-lighthouse-budget.mjs +++ b/scripts/check-lighthouse-budget.mjs @@ -87,6 +87,10 @@ export function incompleteBudgetEvidence(rows, budget, { ignoreBaseline = false // before anything is measured rather than a per-run problem. const problems = new Set(collidingRouteSlugs(budget?.routes ?? []).map((slug) => `route slug collision: ${slug}`)); const byRun = new Map(rows.map((row) => [row.run, row])); + // Browser drift is collected separately from the other problems because it is ONE + // fact about the baseline, not N independent per-route defects — see the collapse + // below. The verdict is identical either way; only the message changes. + const drift = new Map(); for (const run of expectedBudgetRuns(budget)) { const row = byRun.get(run); @@ -123,11 +127,28 @@ export function incompleteBudgetEvidence(rows, budget, { ignoreBaseline = false // version, so a browser bump is otherwise indistinguishable from an application // regression. summarise-web-vitals.mjs makes the same point about its baselines. if (before.chromeVersion && row.chromeVersion && before.chromeVersion !== row.chromeVersion) { - problems.add( - `${run}: baseline measured by a different browser (${before.chromeVersion} vs ${row.chromeVersion}) — refresh with --update`, - ); + drift.set(run, { before: before.chromeVersion, after: row.chromeVersion }); } } + + // One browser bump reds every run in the budget, and printing ten near-identical + // sentences buried the single actionable instruction. Collapse to one line ONLY + // when drift is the whole story and every run drifted the same way — any + // measurement gap, or a mixed set of browsers, still lists per run because those + // are genuinely different facts. This changes the message, never the verdict: + // `compareToLighthouseBudget` still returns `fail` on a non-empty result, + // independently of `enforce`. + const driftPairs = new Set([...drift.values()].map(({ before, after }) => JSON.stringify([before, after]))); + if (drift.size > 0 && problems.size === 0 && driftPairs.size === 1) { + const [{ before, after }] = drift.values(); + return [ + `browser drift on ${drift.size} run(s): the baseline was measured by ${before}, this run used ${after} — ` + + 'refresh it with the CI "Refresh Lighthouse baseline" dispatch (workflow_dispatch, refresh_lighthouse_baseline)', + ]; + } + for (const [run, { before, after }] of drift) { + problems.add(`${run}: baseline measured by a different browser (${before} vs ${after}) — refresh with --update`); + } return [...problems].sort(); } diff --git a/scripts/ci-change-scope.mjs b/scripts/ci-change-scope.mjs index 054fbe7b8d..5018332b21 100644 --- a/scripts/ci-change-scope.mjs +++ b/scripts/ci-change-scope.mjs @@ -24,6 +24,7 @@ const outputs = [ "static_heavy_changed", "coverage_changed", "ui_changed", + "perf_changed", "advisory_ui_changed", "db_changed", "container_changed", @@ -170,6 +171,86 @@ const uiPatterns = [ /^scripts\/(run|check)-lighthouse-budget\.mjs$/, ]; +/** + * perf_changed — surfaces that can plausibly move LCP / TBT / CLS on the five routes + * `lighthouse-budget.json` measures. Deliberately NARROWER than the `ui_changed || + * build_changed` union the budget job used to key off: that union put a dev-dependency + * lockfile bump (#1668, js-yaml) and every `worker/**` ingestion change through a ~7 + * minute isolated `next build --webpack` plus ten Lighthouse runs, with zero + * render-path relevance. + * + * Direction of failure is deliberate: an UNRECOGNISED path under a listed root is IN + * scope. Adding `src/features/` later over-triggers by one job rather than silently + * dropping a render surface out of the perf gate. + */ +const perfPatterns = [ + // Every measured route is a segment of src/app, and the root layout, the + // (search-app) group layout and both CSS entry points ship on all five. src/lib is + // NOT split into server/client here: only 13 of ~256 files carry `import + // "server-only"`, and src/lib/supabase/client.tsx is a browser provider in the + // render tree, so a path-based split would fail open. + "src", + // Route payload, both forms: data/** is imported into route chunks and public/** is + // fetched on the critical path (#117 therapies-home ~136 KB, #013 forms-catalog + // ~132 KB). + "data", + "public", + // These rewrite the emitted bundle/CSS for every route, so a change invalidates the + // whole baseline rather than one route. + "next.config.ts", + "postcss.config.mjs", + "tsconfig.json", + // The budget's own inputs. summarise-web-vitals.mjs is here because + // check-lighthouse-budget.mjs imports summariseReport / hasUsableMetrics / + // measuredRequestedPage from it — editing it changes the VERDICT, and before this + // it matched no CI scope pattern at all. + "lighthouse-budget.json", + /^scripts\/(run-lighthouse-budget|check-lighthouse-budget|summarise-web-vitals|lighthouse-measurement-outcome)\.mjs$/, + // Measuring and refreshing jobs MUST resolve the same Chromium. Editing this action + // changes which browser grades the baseline, so it belongs in perf scope even when + // no application source moved. + ".github/actions/setup-lighthouse-chromium", +]; + +/** + * App Router handlers that budgeted routes fetch during the initial Lighthouse + * navigation. ClinicalDashboard always calls /api/setup-status on mount, and + * readLocalProjectIdentity always calls /api/local-project-id before that. A bare + * URL with no query string does NOT imply "no API on load". + */ +const perfInitialLoadApiPatterns = ["src/app/api/setup-status", "src/app/api/local-project-id"]; + +/** + * Paths inside a `perfPatterns` root that cannot reach a measured route's render + * path. Each is a deliberate loss of coverage; a path NOT listed here stays in scope. + */ +const perfExclusionPatterns = [ + // Most App Router API handlers are off the Lighthouse critical path. Initial-load + // handlers in perfInitialLoadApiPatterns are carved out above and stay in scope. + // assertBudgetRoutesAreQueryFree() still fails closed if a budget route gains a + // query string — that is the usual signal that more handlers may need carving out. + // Precedent: isUiChangedPath already excludes API routes from the UI lane. + "src/app/api", + // Separate route segments, 404 in production, and the runner sets + // NEXT_PUBLIC_MOCKUPS_ENABLED=false. Tailwind does scan src/**, so a utility class + // used only by a mockup adds bytes to the shared stylesheet — tens of bytes against + // an lcpMs floor of +100ms, and check:bundle-budget still enforces gzip growth. + "src/app/mockups", + // Server/edge runtime only. src/instrumentation-client.ts is deliberately NOT here: + // it executes in the browser and is a direct TBT contributor. src/proxy.ts is also + // NOT here: its matcher runs before every budgeted page request (CSP/nonce and + // optional session refresh), so added latency there moves TTFB/LCP directly. + "src/instrumentation.ts", + "src/sentry.server.config.ts", + "src/sentry.edge.config.ts", +]; + +function isPerfChangedPath(filePath) { + if (pathMatches(filePath, perfInitialLoadApiPatterns)) return true; + if (pathMatches(filePath, perfExclusionPatterns)) return false; + return pathMatches(filePath, perfPatterns); +} + // Migration replay validates schema/SQL tooling, not every API handler. API // route edits stay covered by unit/coverage (+ RAG offline when rag-scoped). const dbPatterns = [ @@ -279,6 +360,7 @@ function classify(files, { readLedger = readFlakeLedger, prPolicyBodyPresent = e const docsChanged = normalized.some((file) => pathMatches(file, docPatterns)); const sourceChanged = normalized.some((file) => pathMatches(file, [...sourcePatterns, ...staticConfigPatterns])); const uiChanged = normalized.some((file) => isUiChangedPath(file)); + const perfChanged = normalized.some((file) => isPerfChangedPath(file)); const advisoryUiChanged = normalized.some((file) => pathMatches(file, mockupPatterns)) || quarantineLedgerHasEntries(readLedger); const dbChanged = normalized.some((file) => pathMatches(file, dbPatterns)); @@ -316,6 +398,7 @@ function classify(files, { readLedger = readFlakeLedger, prPolicyBodyPresent = e static_heavy_changed: staticHeavyChanged, coverage_changed: coverageChanged, ui_changed: uiChanged, + perf_changed: perfChanged, advisory_ui_changed: advisoryUiChanged, db_changed: dbChanged, container_changed: containerChanged, @@ -530,6 +613,43 @@ function assertMockupSpecParity() { console.log(`Mockup spec parity: ${specs.length} advisory specs all match mockupPatterns.`); } +/** + * Budget routes stay query-free so a silent `?q=` addition cannot hide new + * client-driven API traffic from review. This is a signal, not a complete proof that + * no API runs on load — `/` already fetches /api/setup-status and + * /api/local-project-id via ClinicalDashboard, and those handlers are carved into + * perfInitialLoadApiPatterns. A new query-bearing route usually means more handlers + * need the same carve-out. Fails CLOSED on an unreadable budget. + */ +function assertBudgetRoutesAreQueryFree() { + let budget; + try { + budget = JSON.parse(readFileSync("lighthouse-budget.json", "utf8")); + } catch (error) { + throw new Error( + `budget-routes-query-free: could not read lighthouse-budget.json (${ + error instanceof Error ? error.message : String(error) + }). If that file moved, update this guard — do not delete it.`, + ); + } + + const routes = Array.isArray(budget?.routes) ? budget.routes : null; + if (!routes || routes.length === 0) { + throw new Error( + "budget-routes-query-free: lighthouse-budget.json lists no routes; this guard has nothing to check.", + ); + } + + const withQuery = routes.filter((route) => typeof route === "string" && route.includes("?")); + if (withQuery.length > 0) { + throw new Error( + `budget-routes-query-free: ${withQuery.join(", ")} carries a query string. A query-bearing budget route may ` + + "fetch additional src/app/api/** handlers on load — extend perfInitialLoadApiPatterns or drop the route.", + ); + } + console.log(`Budget routes query-free: ${routes.length} routes carry no query string.`); +} + function selfTest() { // #137: the advisory lane runs only when it has something to cover. All four // directions matter — a lane that silently never runs is the failure mode. @@ -674,6 +794,128 @@ function selfTest() { ui_changed: true, }, ); + + // ---- perf_changed: the narrow scope the Lighthouse budget job keys off. ---- + // + // Both directions are load-bearing. A false negative means a render regression + // reaches main unmeasured; a false positive is the ~7 minute build+measure this + // scope exists to stop paying. Each `perf-off-*` case below is a deliberate, + // argued loss of coverage, not an oversight — see perfExclusionPatterns. + + // #1668 (dependabot js-yaml, a devDependency) paid the full budget run. This + // classifier only ever sees PATHS, never the lockfile diff, so it cannot tell a + // devDependency bump from a React/Next bump. The PR arm therefore leaves + // perf_changed=false for manifests; the push-to-main arm of the job's `if:` + // re-runs when lockfile_changed is true, and the weekly schedule remains the + // delayed backstop. Do NOT put the lockfile back into perfPatterns. + assertScope("perf-off-for-dependency-manifests", ["package.json", "package-lock.json"], { + build_changed: true, + lockfile_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-worker", ["worker/main.ts", "worker/python/requirements.txt"], { + build_changed: true, + container_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-container-surfaces", ["Dockerfile.worker", "railway.worker.json"], { + container_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-api-route", ["src/app/api/answer/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: false, + }); + // Initial-load handlers the budgeted `/` dashboard always fetches on mount. + assertScope("perf-on-for-initial-load-setup-status", ["src/app/api/setup-status/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: true, + }); + assertScope("perf-on-for-initial-load-local-project-id", ["src/app/api/local-project-id/route.ts"], { + build_changed: true, + ui_changed: false, + perf_changed: true, + }); + // A spec, fixture, golden PNG or browser config cannot change a byte the production + // server sends, and the Lighthouse runner builds and serves its own isolated app + // without ever loading Playwright. + assertScope( + "perf-off-for-playwright-surfaces", + [ + "tests/ui-smoke.spec.ts", + "tests/helpers/zero-touch.ts", + "tests/__screenshots__/linux/dashboard-shell.png", + "playwright.config.ts", + "scripts/run-playwright.mjs", + ], + { ui_changed: true, perf_changed: false }, + ); + assertScope("perf-off-for-mockup-route", ["src/app/mockups/page.tsx"], { + ui_changed: true, + advisory_ui_changed: true, + perf_changed: false, + }); + assertScope( + "perf-off-for-server-runtime-entrypoints", + ["src/instrumentation.ts", "src/sentry.server.config.ts", "src/sentry.edge.config.ts"], + { build_changed: true, perf_changed: false }, + ); + // The request proxy runs before every budgeted navigation (CSP/nonce, optional + // session refresh). Keeping it out of perf scope would miss TTFB/LCP regressions. + assertScope("perf-on-for-request-proxy", ["src/proxy.ts"], { + build_changed: true, + perf_changed: true, + }); + assertScope("perf-off-for-bundle-budget-config", ["bundle-budget.json"], { + build_changed: true, + perf_changed: false, + }); + assertScope("perf-off-for-docs", ["docs/testing.md"], { docs_only: true, perf_changed: false }); + assertScope("perf-off-for-supabase", ["supabase/migrations/20260101000000_example.sql"], { + db_changed: true, + perf_changed: false, + }); + + assertScope("perf-on-for-route-page", ["src/app/(search-app)/dsm/page.tsx"], { + ui_changed: true, + perf_changed: true, + }); + assertScope("perf-on-for-shared-component", ["src/components/clinical-dashboard/dashboard-nav.tsx"], { + perf_changed: true, + }); + // Pins the exclusion that was CONSIDERED AND REJECTED: src/lib is not split into + // server/client by path, because this file is a browser Supabase provider that sits + // in the render tree while its siblings are server-only. + assertScope("perf-on-for-browser-supabase-client", ["src/lib/supabase/client.tsx"], { perf_changed: true }); + // The mirror image: instrumentation-client.ts runs in the browser and is a direct + // TBT contributor, so the `src/instrumentation.ts` exclusion must not over-reach. + assertScope("perf-on-for-client-instrumentation", ["src/instrumentation-client.ts"], { perf_changed: true }); + assertScope("perf-on-for-css-entrypoints", ["src/app/globals.css"], { perf_changed: true }); + assertScope( + "perf-on-for-route-payload", + ["public/therapy-compass-data/therapies-home.json", "data/medications-snapshot.json"], + { perf_changed: true }, + ); + assertScope("perf-on-for-build-config", ["next.config.ts", "postcss.config.mjs", "tsconfig.json"], { + perf_changed: true, + }); + assertScope("perf-on-for-budget-inputs", ["lighthouse-budget.json"], { ui_changed: true, perf_changed: true }); + // New coverage: the grader imports its completeness primitives from this file, so + // editing it changes the verdict. Before perfPatterns it matched no scope at all. + assertScope("perf-on-for-grader-dependency", ["scripts/summarise-web-vitals.mjs"], { perf_changed: true }); + assertScope("perf-on-for-retry-outcome-module", ["scripts/lighthouse-measurement-outcome.mjs"], { + perf_changed: true, + }); + assertScope("perf-on-for-chromium-pin-action", [".github/actions/setup-lighthouse-chromium/action.yml"], { + workflow_changed: true, + perf_changed: true, + }); + // Proves the unknown-path direction: a new top-level directory under src/ is IN + // scope, so a future refactor over-triggers by one job rather than silently + // dropping a render surface. + assertScope("perf-on-for-unrecognised-src-path", ["src/features/new-thing/index.ts"], { perf_changed: true }); assertScope("runtime-data", ["data/medications-snapshot.json"], { source_changed: true, coverage_changed: true, @@ -881,6 +1123,10 @@ function selfTest() { static_heavy_changed: true, coverage_changed: true, ui_changed: true, + // The weekly schedule and any unresolvable base resolve to these sentinels, and + // the perf gate's `if:` relies on that to keep measuring routes when no PR does. + // Asserted so a future edit to fullRunSentinelFiles cannot silently retire it. + perf_changed: true, db_changed: true, container_changed: true, rag_eval_changed: true, @@ -907,6 +1153,7 @@ function selfTest() { const args = process.argv.slice(2); if (args.includes("--self-test")) { assertMockupSpecParity(); + assertBudgetRoutesAreQueryFree(); selfTest(); process.exit(0); } diff --git a/scripts/lighthouse-measurement-outcome.mjs b/scripts/lighthouse-measurement-outcome.mjs new file mode 100644 index 0000000000..8c4b46ca96 --- /dev/null +++ b/scripts/lighthouse-measurement-outcome.mjs @@ -0,0 +1,60 @@ +/** + * lighthouse-measurement-outcome — decide whether a Lighthouse cell produced + * gradeable evidence at all, and therefore whether one retry is warranted. + * + * Why this is its own module: `run-lighthouse-budget.mjs` executes its whole pipeline + * at import time (it builds, serves and measures at the top level), so it cannot be + * imported by a test. Putting the decision here keeps it unit-testable without the + * source-text assertions this repo has been retiring. + * + * The distinction that matters: + * + * - A measurement that never STARTED is not evidence of a regression. Lighthouse's + * own `NO_NAVSTART` runtime error means the navigation never began, and its own + * advice is to run again; ledger #147 recorded `/forms` doing exactly this while + * the live dispatch measured it fine. + * - A measurement that DID start and produced bad numbers is a real measurement. + * It is never retried — that would be the gate re-rolling the dice until green. + * + * This only decides whether to retry. It never decides whether a run passes: the + * grader's `incompleteBudgetEvidence` still fails closed on a missing or unusable + * report regardless of `enforce`, before and after any retry. + * + * Note the second failure shape, which the runner previously missed entirely: + * Lighthouse exits 0 and writes a perfectly well-formed report whose only content is + * a `runtimeError`. That file satisfies "a report exists" while carrying no metrics, + * so an exit-code check alone leaves it unretried. + */ + +/** + * Why this cell produced no gradeable evidence, or `null` when it did. + * + * @param {number | null | undefined} exitCode Lighthouse's process exit code. + * @param {string | null | undefined} reportText Raw contents of the report file, or + * `null`/`undefined` when no file was written. + * @returns {string | null} A short human-readable reason, or `null` when the cell + * measured something real (including something real and slow). + */ +export function measurementFailureReason(exitCode, reportText) { + if (typeof exitCode !== "number" || exitCode !== 0) { + return `lighthouse exited ${exitCode ?? "without a status"}`; + } + if (reportText === null || reportText === undefined || reportText === "") { + return "no report file was written"; + } + + let parsed; + try { + parsed = JSON.parse(reportText); + } catch { + return "report is not valid JSON"; + } + + const code = parsed?.runtimeError?.code; + // Any runtimeError, not just NO_NAVSTART: `measuredRequestedPage` in + // summarise-web-vitals.mjs already rejects every report carrying one, so each is a + // cell that produced no comparable numbers. + if (typeof code === "string" && code.length > 0) return `lighthouse runtimeError ${code}`; + + return null; +} diff --git a/scripts/run-lighthouse-budget.mjs b/scripts/run-lighthouse-budget.mjs index 9e8ff1ce83..5eede6b2c6 100644 --- a/scripts/run-lighthouse-budget.mjs +++ b/scripts/run-lighthouse-budget.mjs @@ -21,11 +21,20 @@ * live workflow, which is grading a flaky public network): a route that produced no * report is incomplete evidence, and the grader fails closed on it. * + * A cell that produced no measurement AT ALL — a non-zero exit, no report file, or a + * report carrying only a `runtimeError` such as Lighthouse's own `NO_NAVSTART` — gets + * exactly ONE announced retry first (`lighthouse-measurement-outcome.mjs` draws that + * line). That is not a softening: a run that never started measured nothing about this + * diff, and if the retry also produces nothing the grader still fails closed. Every + * retry is reported to the run summary and written to `retries.txt` whether or not it + * recovered, so a chronically flaky route cannot hide behind a green run. A cell that + * DID measure and produced bad numbers is never retried. + * * Flags: --dry-run (print the plan and exit), --update (refresh the baseline), * --keep (leave reports in place), --dir . */ import { spawn, spawnSync } from "node:child_process"; -import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { appendFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import http from "node:http"; import net from "node:net"; import path from "node:path"; @@ -35,6 +44,7 @@ import { childProcessExitCode, childProcessFailureSummary } from "./child-proces import { offlineTestEnvironment } from "./test-environment.mjs"; import { acquireHeavyRunLock } from "./test-run-lock.mjs"; import { loadBudget } from "./check-lighthouse-budget.mjs"; +import { measurementFailureReason } from "./lighthouse-measurement-outcome.mjs"; import { appName, circularProjectPortRange, @@ -312,42 +322,83 @@ try { const chromePath = process.env.CHROME_PATH ?? process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH ?? ""; const failures = []; + const retried = []; + + const measure = (strategy, route, output) => + spawnSync( + npxInvocation.command, + [ + ...npxInvocation.prefixArgs, + "--yes", + `lighthouse@${LIGHTHOUSE_VERSION}`, + `${baseUrl}${route}`, + "--output=json", + `--output-path=${output}`, + `--preset=${strategy === "desktop" ? "desktop" : "perf"}`, + "--only-categories=performance", + "--chrome-flags=--headless=new --no-sandbox --disable-dev-shm-usage", + "--max-wait-for-load=60000", + "--quiet", + ], + { + cwd: projectRoot, + env: { ...offlineEnv, ...(chromePath ? { CHROME_PATH: chromePath } : {}) }, + stdio: "inherit", + }, + ); + + const readIfPresent = (file) => (existsSync(file) ? readFileSync(file, "utf8") : null); for (const strategy of strategies) { for (const route of routes) { + const cell = `${strategy} ${route}`; const output = path.join(reportDirectory, `${strategy}-${slugFor(route)}.json`); - console.log(`Measuring ${strategy} ${route}`); - const result = spawnSync( - npxInvocation.command, - [ - ...npxInvocation.prefixArgs, - "--yes", - `lighthouse@${LIGHTHOUSE_VERSION}`, - `${baseUrl}${route}`, - "--output=json", - `--output-path=${output}`, - `--preset=${strategy === "desktop" ? "desktop" : "perf"}`, - "--only-categories=performance", - "--chrome-flags=--headless=new --no-sandbox --disable-dev-shm-usage", - "--max-wait-for-load=60000", - "--quiet", - ], - { - cwd: projectRoot, - env: { ...offlineEnv, ...(chromePath ? { CHROME_PATH: chromePath } : {}) }, - stdio: "inherit", - }, - ); - // Not downgraded to a warning: the grader treats a missing report as - // incomplete evidence and fails, which is the behaviour we want locally too. - if (childProcessExitCode(result) !== 0) { - const failure = `${strategy} ${route}`; - failures.push(failure); - console.log(`::warning::lighthouse ${failure} failed (${childProcessFailureSummary(result)})`); + console.log(`Measuring ${cell}`); + let result = measure(strategy, route, output); + let reason = measurementFailureReason(childProcessExitCode(result), readIfPresent(output)); + + if (reason) { + // ONE retry, and it is announced. A cell that never started measured nothing + // about this diff, so treating it as a regression is wrong — but so is + // treating it as a pass, which is why nothing below is downgraded. If the + // retry also produces no measurement the report stays missing or errored and + // the grader's incompleteBudgetEvidence still fails closed regardless of + // `enforce`. The retry is reported either way, so a chronically flaky route + // cannot hide behind a green run. + console.log(`::warning::lighthouse ${cell} produced no measurement (${reason}); retrying once`); + // Never leave the first attempt's file behind: a runtimeError report would + // otherwise be graded, or baked into a refreshed baseline, if the retry fails + // before writing. + rmSync(output, { force: true }); + result = measure(strategy, route, output); + const after = measurementFailureReason(childProcessExitCode(result), readIfPresent(output)); + retried.push(`${cell} (${reason}${after ? ` -> still ${after}` : " -> recovered"})`); + reason = after; + } + + if (reason) { + failures.push(cell); + console.log( + `::warning::lighthouse ${cell} failed after one retry (${reason}; ${childProcessFailureSummary(result)})`, + ); } } } + // Emitted whether or not the retry recovered: a retry is a fact about the evidence + // and belongs in the run summary and the artifact, not only in the scrollback. + if (retried.length > 0) { + const line = `lighthouse retried ${retried.length} cell(s): ${retried.join("; ")}`; + console.log(`::warning::${line}`); + if (process.env.GITHUB_STEP_SUMMARY) { + appendFileSync(process.env.GITHUB_STEP_SUMMARY, `\n> Lighthouse ${line}\n`); + } + // Deliberately .txt, not .json: `readReports` in check-lighthouse-budget.mjs + // globs *.json and skips only summary.json, so a JSON sidecar here would be + // parsed as a Lighthouse report and become a phantom row named `retries`. + writeFileSync(path.join(reportDirectory, "retries.txt"), `${retried.join("\n")}\n`, "utf8"); + } + if (failures.length > 0) console.log(`::warning::lighthouse failed for ${failures.join(", ")}`); // --require-reports: this runner OWNS the directory and has just tried to measure diff --git a/tests/check-lighthouse-budget.test.ts b/tests/check-lighthouse-budget.test.ts index 9f6e1d719b..50579a75d4 100644 --- a/tests/check-lighthouse-budget.test.ts +++ b/tests/check-lighthouse-budget.test.ts @@ -1,4 +1,5 @@ -import { readFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; @@ -10,8 +11,10 @@ import { expectedBudgetRuns, gradeRun, incompleteBudgetEvidence, + readReports, renderBudgetTable, } from "../scripts/check-lighthouse-budget.mjs"; +import { measurementFailureReason } from "../scripts/lighthouse-measurement-outcome.mjs"; /** Kept in step with lighthouse-budget.json. */ const ROUTES = ["/", "/therapy-compass", "/documents/search", "/dsm", "/forms"]; @@ -133,13 +136,60 @@ describe("incompleteBudgetEvidence — completeness derived from what is graded" expect(result.reason).toBe("evidence incomplete"); }); - it("rejects a baseline measured by a different browser", () => { + it("rejects a baseline measured by a different browser, collapsed to one instruction", () => { + // One browser bump reds every run in the budget. Ten near-identical sentences + // buried the single actionable line, so drift collapses to one message when it is + // the whole story — the VERDICT is unchanged and asserted below. const rows = completeRows(); const stale = baselineFromRows(rows.map((entry: Row) => ({ ...entry, chromeVersion: "HeadlessChrome/131" }))); const problems = incompleteBudgetEvidence(rows, budget({ baseline: stale })); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("browser drift on 10 run(s)"); + expect(problems[0]).toContain("HeadlessChrome/131"); + expect(problems[0]).toContain("HeadlessChrome/140"); + expect(problems[0]).toContain("Refresh Lighthouse baseline"); + + // The collapse is cosmetic. Incomplete evidence still fails closed, and still + // does so independently of `enforce`. + for (const enforce of [true, false]) { + const result = compareToLighthouseBudget(rows, budget({ baseline: stale, enforce })); + expect(result.status).toBe("fail"); + expect(result.reason).toBe("evidence incomplete"); + } + }); + + it("lists drift per run when the browsers themselves disagree", () => { + // Two different baseline browsers is not one fact, so it must not read as one. + const rows = completeRows(); + const mixed = Object.fromEntries( + Object.entries(baselineFromRows(rows)).map(([run, entry], index) => [ + run, + { ...(entry as object), chromeVersion: index % 2 === 0 ? "HeadlessChrome/131" : "HeadlessChrome/132" }, + ]), + ); + const problems = incompleteBudgetEvidence(rows, budget({ baseline: mixed })); + expect(problems).toHaveLength(10); - expect(problems[0]).toContain("measured by a different browser"); + expect(problems.every((problem: string) => problem.includes("measured by a different browser"))).toBe(true); + }); + + it("keeps drift per run when a measurement gap shares the verdict", () => { + // A missing report and a browser bump are different facts with different fixes; + // collapsing here would hide the one that --update cannot resolve. + const rows = completeRows().filter((entry: Row) => entry.run !== "mobile-dsm"); + const stale = baselineFromRows( + completeRows().map((entry: Row) => ({ ...entry, chromeVersion: "HeadlessChrome/131" })), + ); + const problems = incompleteBudgetEvidence(rows, budget({ baseline: stale })); + + expect(problems).toEqual( + expect.arrayContaining([ + "mobile-dsm: no Lighthouse report produced", + expect.stringContaining("measured by a different browser"), + ]), + ); + expect(problems.length).toBeGreaterThan(1); }); it("ignores browser drift and missing baseline rows when refreshing", () => { @@ -343,6 +393,14 @@ describe("committed lighthouse-budget.json", () => { expect(committed.strategies).toEqual(["mobile", "desktop"]); }); + it("measures every route without a query string", () => { + // Budget routes stay query-free so a silent `?q=` addition cannot hide new + // client-driven API traffic. This is a signal, not a complete proof that no API + // runs on load — `/` already fetches /api/setup-status and /api/local-project-id, + // and those handlers are carved into perfInitialLoadApiPatterns. + expect(committed.routes.filter((route) => route.includes("?"))).toEqual([]); + }); + it("invokes npm's JavaScript npx CLI through Node when available", () => { const runner = readFileSync(path.join(process.cwd(), "scripts", "run-lighthouse-budget.mjs"), "utf8"); @@ -355,6 +413,71 @@ describe("committed lighthouse-budget.json", () => { }); }); +describe("measurementFailureReason", () => { + const report = (extra: Record = {}) => + JSON.stringify({ requestedUrl: "http://localhost:4461/forms", audits: {}, ...extra }); + + it("passes a clean run through", () => { + expect(measurementFailureReason(0, report())).toBeNull(); + }); + + it("flags a non-zero exit", () => { + expect(measurementFailureReason(1, null)).toContain("exited 1"); + }); + + it("flags a run that was killed without a status", () => { + expect(measurementFailureReason(null, null)).toContain("without a status"); + }); + + it("flags a clean exit that wrote no report", () => { + expect(measurementFailureReason(0, null)).toBe("no report file was written"); + expect(measurementFailureReason(0, "")).toBe("no report file was written"); + }); + + it("flags an unparseable report", () => { + expect(measurementFailureReason(0, "{not json")).toBe("report is not valid JSON"); + }); + + it("flags the NO_NAVSTART shape that exits zero with a well-formed report", () => { + // Ledger #147: `/forms` did this locally while the live dispatch measured it + // fine. An exit-code check alone leaves it unretried, because Lighthouse both + // exits 0 and writes a valid file whose only content is the runtime error. + expect(measurementFailureReason(0, report({ runtimeError: { code: "NO_NAVSTART" } }))).toBe( + "lighthouse runtimeError NO_NAVSTART", + ); + }); + + it("never retries a real measurement that produced bad numbers", () => { + // The line that keeps this a retry and not a re-roll: a page that loaded and + // scored badly is evidence, and re-running it until it goes green is the failure + // mode this whole gate exists to prevent. + const slow = report({ audits: { "largest-contentful-paint": { numericValue: 9999 } } }); + + expect(measurementFailureReason(0, slow)).toBeNull(); + }); +}); + +describe("readReports", () => { + it("ignores the retry sidecar rather than reading it as a report", () => { + // retries.txt is deliberately not .json: readReports globs *.json and skips only + // summary.json, so a JSON sidecar would be parsed as a Lighthouse report and + // become a phantom row named `retries` — a run the budget never asked for, + // carrying no metrics. + const directory = mkdtempSync(path.join(tmpdir(), "lighthouse-reports-")); + try { + writeFileSync( + path.join(directory, "mobile-root.json"), + JSON.stringify({ requestedUrl: "http://localhost:4461/", finalUrl: "http://localhost:4461/", audits: {} }), + ); + writeFileSync(path.join(directory, "retries.txt"), "mobile /forms (lighthouse runtimeError NO_NAVSTART)\n"); + + expect(readReports(directory).map((entry: { run: string }) => entry.run)).toEqual(["mobile-root"]); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); +}); + describe("renderBudgetTable", () => { it("subordinates every measurement when the evidence is incomplete", () => { const rows = completeRows().filter((entry: Row) => entry.run !== "mobile-forms"); diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index d1c4d074eb..444d39912d 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -308,3 +308,94 @@ describe.skipIf(process.platform === "win32")("PR required aggregate — cancell expect(offenders).toEqual([]); }); }); + +describe("Lighthouse budget routing", () => { + /** The `lighthouse-budget:` block, up to the next top-level job key. */ + const lighthouseJob = /\n lighthouse-budget:\n([\s\S]*?)(?=\n [a-z][\w-]*:\n)/.exec(workflow)?.[1] ?? ""; + const refreshJob = /\n lighthouse-baseline-refresh:\n([\s\S]*?)(?=\n [a-z][\w-]*:\n)/.exec(workflow)?.[1] ?? ""; + + it("finds both Lighthouse jobs", () => { + // Fails closed on a rename rather than turning every assertion below into a + // vacuous match against an empty string. + expect(lighthouseJob, "lighthouse-budget job not found in ci.yml").not.toBe(""); + expect(refreshJob, "lighthouse-baseline-refresh job not found in ci.yml").not.toBe(""); + }); + + it("exports perf_changed from the change-scope job", () => { + expect(workflow).toContain("perf_changed: ${{ steps.scope.outputs.perf_changed }}"); + }); + + it("keys the budget off perf scope, not the old ui/build union", () => { + // `ui_changed || build_changed` put every dependabot lockfile bump and every + // worker/** change through a ~7 minute build plus ten Lighthouse runs. + expect(lighthouseJob).toContain("needs.changes.outputs.perf_changed == 'true'"); + expect(lighthouseJob).not.toContain("needs.changes.outputs.ui_changed"); + expect(lighthouseJob).not.toContain("needs.changes.outputs.build_changed"); + }); + + it("re-runs Lighthouse on push when the lockfile changed", () => { + // perf_changed deliberately stays false for package.json / package-lock.json + // (paths cannot distinguish a React bump from a js-yaml bump). Without this + // push arm, a lockfile-only merge would skip Lighthouse on the PR and again + // on the push to main, leaving only the weekly schedule. + expect(lighthouseJob).toContain("github.event_name == 'push'"); + expect(lighthouseJob).toContain("needs.changes.outputs.lockfile_changed == 'true'"); + }); + + it("tests draft with `!= true`, so push and schedule runs survive", () => { + // `github.event.pull_request` is null on push/schedule/merge_group, so + // `draft == false` is FALSE there and would silently kill both arms. + expect(lighthouseJob).toContain("github.event.pull_request.draft != true"); + expect(lighthouseJob).not.toContain("github.event.pull_request.draft == false"); + }); + + it("reads the dispatch input through github.event.inputs, which is null off-dispatch", () => { + // The `inputs` context only exists for workflow_dispatch/workflow_call; the + // github.event.inputs form is a string and is safely null everywhere else. + expect(lighthouseJob).toContain("github.event.inputs.refresh_lighthouse_baseline != 'true'"); + expect(refreshJob).toContain("github.event.inputs.refresh_lighthouse_baseline == 'true'"); + expect(workflow).toMatch(/workflow_dispatch:\n\s+inputs:\n\s+refresh_lighthouse_baseline:/); + }); + + it("pairs promotion to pr-required with merge_group coverage", () => { + // The budget skips merge_group ONLY because it is advisory and outside + // pr-required, where it could add ~7 minutes of merge latency without ever + // changing the outcome. Promoting it (#118) without restoring merge_group would + // leave the queue running a required check the PR never re-verified. + const prRequiredNeeds = /\n pr-required:\n[\s\S]*?needs:\s*\n?\s*\[([\s\S]*?)\]/.exec(workflow)?.[1] ?? ""; + // Fail closed on a lost anchor: an empty match would silently make this guard + // conclude "not required" forever, which is the branch that checks the least. + expect(prRequiredNeeds, "could not read pr-required's needs list from ci.yml").not.toBe(""); + expect(prRequiredNeeds).toContain("static-pr"); + const isRequired = /\blighthouse-budget\b/.test(prRequiredNeeds); + + if (isRequired) { + expect(lighthouseJob, "lighthouse-budget is required — it must also run in merge_group").toContain("merge_group"); + expect(lighthouseJob, "a required check must not be continue-on-error").not.toContain("continue-on-error: true"); + } else { + expect(lighthouseJob).toContain("continue-on-error: true"); + } + }); + + it("keeps the baseline refresh dispatch-only, red on failure, and unable to push", () => { + // A workflow that can rewrite a gate's own baseline is a gate that can green + // itself, so this job only ever produces an artifact for a human to commit. + expect(refreshJob).toContain("github.event_name == 'workflow_dispatch'"); + expect(refreshJob).not.toContain("continue-on-error"); + expect(refreshJob).not.toContain("git push"); + expect(refreshJob).not.toContain("persist-credentials: true"); + // An empty artifact would look like a successful refresh that recorded nothing. + expect(refreshJob).toContain("if-no-files-found: error"); + expect(refreshJob).toContain("--update"); + }); + + it("pins Chromium through one shared action in both jobs", () => { + // If the measuring and refreshing jobs resolve different browsers, the refreshed + // baseline records a browser other than the one grading against it and the gate + // goes permanently red — the exact failure this action was extracted to end. + expect(lighthouseJob).toContain("uses: ./.github/actions/setup-lighthouse-chromium"); + expect(refreshJob).toContain("uses: ./.github/actions/setup-lighthouse-chromium"); + expect(lighthouseJob).not.toContain("playwright install"); + expect(refreshJob).not.toContain("playwright install"); + }); +});