ci(lighthouse): make the budget gate self-healing and scope it to perf-relevant diffs - #1722
Conversation
…f-relevant diffs The Lighthouse budget check has been red on every pull request. It is a relative gate, and 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. PR #1716 pinned Playwright's managed Chromium (HeadlessChrome/151) so the browser stops varying per runner, but the committed baseline still records HeadlessChrome/150 from the old ambient runner Chrome, so all ten cells fail "evidence incomplete" every run. Three changes, none of which weaken the verdict: - Extract the Chromium pin into .github/actions/setup-lighthouse-chromium and use it from both Lighthouse jobs. If the measuring and refreshing jobs resolve different browsers, a refreshed baseline records a browser other than the one grading against it, and the gate goes permanently red again. - Add a dispatch-only lighthouse-baseline-refresh job behind the new refresh_lighthouse_baseline workflow_dispatch input. The baseline is a browser-specific artefact, so it can only be produced on a runner; this job measures under the pinned Chromium and uploads a rewritten lighthouse-budget.json for a human to review and commit. It deliberately cannot push, is not continue-on-error, and prints the distinct chromeVersion values as its acceptance check. - Retry a cell that produced no measurement at all exactly once. The failure has two shapes and only one was detected: Lighthouse can also exit 0 and write a well-formed report whose only content is a runtimeError such as NO_NAVSTART, which is what /forms does intermittently (ledger #147). A run that never started measured nothing about the diff, but it is not a pass either, so 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. scripts/lighthouse-measurement-outcome.mjs owns that decision so it is unit testable; a cell that did measure and produced bad numbers is never retried. Browser drift now collapses to one message naming the refresh dispatch instead of ten near-identical sentences, but only when drift is the whole story and every run drifted the same way. A measurement gap or a mixed set of browsers still lists per run. compareToLighthouseBudget still returns fail, independently of enforce. Separately, the job keyed off `ui_changed || build_changed`, which put every dependabot lockfile bump (#1668, js-yaml) and every worker/** ingestion change through a ~7 minute isolated production build plus ten Lighthouse runs. It now keys off a new, narrower perf_changed scope that excludes worker and container surfaces, dependency manifests, Playwright and test surfaces, src/app/api/** and src/app/mockups/**. 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. The lockfile exclusion is the one deliberate hole: ci-change-scope.mjs sees paths, never the lockfile diff, so it cannot tell a devDependency bump from a React/Next bump. It is covered by check:bundle-budget on the same pull requests and by the push-to-main arm of the job's condition. enforce stays false and the job stays out of pr-required; promoting it is ledger #118's separate staged work, and ci-cache-safety.test.ts now fails if that promotion happens without restoring merge_group to the job's condition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds pinned Chromium setup, narrows Lighthouse execution through ChangesLighthouse CI hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant ci_change_scope
participant lighthouse_budget
participant setup_lighthouse_chromium
participant Lighthouse
CI->>ci_change_scope: classify changed paths
ci_change_scope-->>CI: perf_changed
CI->>setup_lighthouse_chromium: install pinned Chromium
CI->>lighthouse_budget: run performance budget
lighthouse_budget->>Lighthouse: measure route and strategy cell
Lighthouse-->>lighthouse_budget: report and exit status
lighthouse_budget->>Lighthouse: retry failed measurement once
lighthouse_budget-->>CI: budget result and retry summary
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d425e1aa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch claude/lighthouse-ci-smart-gate at starting commit 3d425e1; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:claude/lighthouse-ci-smart-gate, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
@cursoragent hello |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Restore the documented push-to-main lockfile backstop, keep the request proxy and initial-load API handlers in perf_changed, and include the shared Chromium pin action so measure/refresh drift stays in scope. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
PR 1722 review-and-fix summaryTip: Fixed (pushed)Commit
Dispositioned / threads
Required CIRe-triggered by the fix push on Local gates (decisive lines)
Not run: Residual risks
Merge left to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/run-lighthouse-budget.mjs (1)
327-348: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider bounding each Lighthouse invocation with a
spawnSynctimeout.
--max-wait-for-load=60000bounds page load only. It does not bound the Lighthouse process lifetime. A hung Chromium therefore blocks until the 45 minute job timeout, which kills the job before the report upload step runs. The retry doubles this exposure for a failing cell.A per-invocation
timeoutturns a hang into one classified cell failure, which the retry and fail-closed grading already handle.♻️ Proposed timeout bound
{ cwd: projectRoot, env: { ...offlineEnv, ...(chromePath ? { CHROME_PATH: chromePath } : {}) }, stdio: "inherit", + // Chromium can hang past --max-wait-for-load. Bound the process so one cell + // fails and is retried instead of consuming the whole job timeout. + timeout: 5 * 60_000, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-lighthouse-budget.mjs` around lines 327 - 348, Update the spawnSync options in the measure function to include a per-invocation timeout that bounds the Lighthouse process lifetime, while preserving the existing environment, working directory, and stdio settings. Ensure timed-out invocations return a failure result that the existing retry and fail-closed grading logic can classify and handle.scripts/ci-change-scope.mjs (1)
905-910: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning the negative direction for the Lighthouse script regex.
The regex at line 208 is anchored, so only the four named scripts match. No self-test asserts that a sibling script stays out of perf scope. If a later edit relaxes the anchor to a prefix, every
scripts/*.mjschange would trigger the ~7 minute build plus ten Lighthouse runs, and no test would fail.♻️ Proposed negative case
assertScope("perf-on-for-retry-outcome-module", ["scripts/lighthouse-measurement-outcome.mjs"], { perf_changed: true, }); + // The regex is anchored to four named scripts. A sibling script must stay out, or + // every scripts/ edit pays the full build-and-measure run. + assertScope("perf-off-for-unrelated-script", ["scripts/run-vitest.mjs"], { perf_changed: false });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci-change-scope.mjs` around lines 905 - 910, Add a negative self-test alongside the existing perf scope assertions in scripts/ci-change-scope.mjs, using a sibling scripts/*.mjs file that is not one of the four intended Lighthouse scripts and asserting perf_changed is false. Keep the test focused on preventing the Lighthouse regex from matching broader script prefixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 844-849: Update the “Show the baseline diff and the browser it was
measured on” step to assert that the distinct baseline chromeVersion count is
exactly one and exit nonzero otherwise. Preserve the existing diff/version
output, but make the Node validation fail the workflow for zero or multiple
distinct versions.
In `@scripts/check-lighthouse-budget.mjs`:
- Around line 130-151: The browser-drift aggregation in the budget comparison
flow must require drift coverage for every expected run, in addition to a single
drift pair and no measurement problems; otherwise retain per-run diagnostics.
Add a regression test covering a version-less legacy baseline row. Update
docs/testing.md lines 150-152 to document that mixed, partial, or incomplete
evidence remains reported per run.
---
Nitpick comments:
In `@scripts/ci-change-scope.mjs`:
- Around line 905-910: Add a negative self-test alongside the existing perf
scope assertions in scripts/ci-change-scope.mjs, using a sibling scripts/*.mjs
file that is not one of the four intended Lighthouse scripts and asserting
perf_changed is false. Keep the test focused on preventing the Lighthouse regex
from matching broader script prefixes.
In `@scripts/run-lighthouse-budget.mjs`:
- Around line 327-348: Update the spawnSync options in the measure function to
include a per-invocation timeout that bounds the Lighthouse process lifetime,
while preserving the existing environment, working directory, and stdio
settings. Ensure timed-out invocations return a failure result that the existing
retry and fail-closed grading logic can classify and handle.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87f9b08a-b603-4819-bbb0-cbf1aaca2c3b
📒 Files selected for processing (11)
.github/actions/setup-lighthouse-chromium/action.yml.github/workflows/ci.ymldocs/process-hardening.mddocs/scripts-index.mddocs/testing.mdscripts/check-lighthouse-budget.mjsscripts/ci-change-scope.mjsscripts/lighthouse-measurement-outcome.mjsscripts/run-lighthouse-budget.mjstests/check-lighthouse-budget.test.tstests/ci-cache-safety.test.ts
2 participants
Summary
Fix the permanently-red Lighthouse budget. The gate is relative, and
check-lighthouse-budget.mjsfails closed when a baseline row'schromeVersiondiffers from the measuring run's, because a browser bump is otherwise indistinguishable from an application regression. fix(ci): pin Lighthouse's Chrome so the budget gate isn't runner-fleet noise #1716 pinned Playwright's managed Chromium (HeadlessChrome/151) so the browser stops varying per runner, but the committed baseline still records HeadlessChrome/150 from the old ambient runner Chrome. All ten cells therefore fail "evidence incomplete" on every pull request. Decisive line from run 31245151534 (PR fix(safety-plan): clear copy-feedback timer on unmount #1717):Extract the Chromium pin into
.github/actions/setup-lighthouse-chromiumand use it from both Lighthouse jobs. This is load-bearing: if the measuring and refreshing jobs resolve different browsers, a refreshed baseline records a browser other than the one grading against it, and the gate goes permanently red again.Add a dispatch-only
lighthouse-baseline-refreshjob behind a newrefresh_lighthouse_baselineworkflow_dispatchinput. The baseline is a browser-specific artefact, so it can only be produced on a runner (RAG optimisation: answer quality (P1-P10), v17 numeric grounding, offline fallback, hybrid RPC hardening + golden-recall regression fix #118: never commit baselines from a developer machine). The job measures under the pinned Chromium and uploads a rewrittenlighthouse-budget.jsonfor a human to review and commit. It deliberately cannot push, is notcontinue-on-error, and prints the distinctchromeVersionvalues as its acceptance check — a workflow that can rewrite a gate's own baseline is a gate that can green itself.Retry a cell that produced no measurement, exactly once. The failure has two shapes and only one was detected: Lighthouse can also exit 0 and write a well-formed report whose only content is a
runtimeErrorsuch asNO_NAVSTART, which is what/formsdoes intermittently (ledger perf: SSR-safe lazy loading, loading skeletons, and SourceImage memo #147, and observed in CI on PR Speed up site testing selection and CI Production UI shards #1686 asmobile-forms: report has no LCP or CLS number). A run that never started measured nothing about the diff, but it is not a pass either, so if the retry also produces nothing the grader still fails closed. Every retry is reported to the run summary and written toretries.txtwhether 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 — that would be the gate re-rolling the dice until green.scripts/lighthouse-measurement-outcome.mjsowns that decision so it is unit-testable rather than asserted against source text.Collapse the drift message, not the verdict. Browser drift now emits one line naming the refresh dispatch instead of ten near-identical sentences — but only when drift is the whole story and every run drifted the same way. A measurement gap, or a mixed set of browsers, still lists per run because those are genuinely different facts.
compareToLighthouseBudgetstill returnsfail, independently ofenforce, and that is asserted in both directions.Scope the job to perf-relevant diffs. It keyed off
ui_changed || build_changed, which put every dependabot lockfile bump (chore(deps-dev): bump js-yaml from 4.3.0 to 4.3.1 #1668, js-yaml) and everyworker/**ingestion change through a ~7 minute isolated production build plus ten Lighthouse runs, with zero render-path relevance. A new, narrowerperf_changedscope excludes worker and container surfaces, dependency manifests, Playwright/test surfaces including committed screenshots,src/app/api/**, andsrc/app/mockups/**. 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.merge_groupis skipped while the job is advisory;pushtomainalways runs on perf scope.Two exclusions were considered and rejected, and are pinned by self-tests so they are not silently adopted later:
src/lib/**(only 13 of ~256 files areserver-only, andsrc/lib/supabase/client.tsxis a browser provider in the render tree, so a path-based split fails open) and*-mockup?.tsxcomponent names (global-mockup-search-shell.tsxshows the naming is not a boundary — only thesrc/app/mockupsroute segment is excluded).Known limitation, stated rather than papered over. The
package-lock.jsonexclusion is a deliberate hole:ci-change-scope.mjsonly ever sees paths, never the lockfile diff, so it cannot tell a devDependency bump from a React/Next bump. It is covered bycheck:bundle-budget, which is already enforced on those same pull requests, and by thepush-to-mainarm of the job's condition, which re-measures within one merge. If a runtime dependency bump ever lands an unnoticed LCP regression, the fix is to teach the classifier to read the lockfile diff — not to put the whole lockfile back into perf scope. Separately,on.pull_request.typeshas nolabeled, so the newlighthouse-budgetopt-in label takes effect on the next push or re-run; addinglabeledwould re-run all of CI on every label change, soworkflow_dispatchis the immediate escape hatch.This PR does not by itself turn the check green. It ships the machinery. The baseline is still stranded on HeadlessChrome/150, and the fix is a separate, human-reviewed commit of
lighthouse-budget.jsonproduced by the new refresh dispatch on this branch.RAG impact: no retrieval behaviour change — this PR touches only CI workflow routing, the Lighthouse budget runner and grader, their tests, and documentation. No file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking-config, answer-ranking, the eval harness, the golden fixture, or the retrieval RPCs is modified, andclassifyPullRequestFilesreportsragRanking: falsefor this diff.Verification
npm run check:ci-scope—Mockup spec parity: 5 advisory specs all match mockupPatterns./Budget routes query-free: 5 routes carry no query string./CI change scope self-test passed.npx vitest run tests/check-lighthouse-budget.test.ts—Tests 49 passed (49)npm run test:ci-workflows—Test Files 12 passed (12),Tests 232 passed | 11 skipped (243)npm run verify:lighthouse -- --dry-run— plan prints all ten cells, exit 0npm run check:github-actions—GitHub Actions pin check passed.(covers the new composite action's pinned SHAs)npm run format— clean;npx prettier --check scripts/check-lighthouse-budget.mjs—All matched files use Prettier code style!npm run verify:cheap—Test Files 2 failed | 521 passed (523),Tests 6 failed | 5584 passed | 14 skipped (5604)Both failing files are pre-existing Windows-local defects, unrelated to this diff, and this diff touches zero files either of them reads:
tests/mode-nav-addon-slot.dom.test.tsx— line 118 doespath.replace(\${process.cwd()}/`, ""), a POSIX-only strip. On Windowsjoin()yields backslashes, so the prefix never matches and absolute paths survive into the assertion. It walks onlysrc/components/**; this diff changes no file undersrc/`.tests/pr-handoff-stop.test.ts(5 failures, all in the "when jq is unavailable" group) — spawnsbash .claude/hooks/pr-handoff-stop.sh; this diff changes no file under.claude/.Main's own CI run for
d136991afreportsUnit coverage | success, so both pass on the Linux runners. An earlier local run also failedtests/private-access-routes.test.ts, which passed in isolation (150 passed) and did not recur — a parallel-run flake, recorded here rather than omitted.Not run, and why:
npm run verify:ui/verify:phone-chrome— no UI surface changes;classifyPullRequestFilesreportsui: false.npm run verify:lighthouse— needs a pinned Chromium this Windows environment does not provide, and local LCP is not comparable to CI anyway (perf: SSR-safe lazy loading, loading skeletons, and SourceImage memo #147). The measurement path is covered by themeasurementFailureReasonunit table; the integration proof is this PR's own CI run.offlineTestEnvironment,PLAYWRIGHT_OFFLINE_MODE=true).Risk and rollout
enforcestaysfalseand the job stayscontinue-on-errorand outsidepr-required, so nothing here can newly block a merge; the failure modes are "the advisory job runs when it need not" or "does not run when it could have". The one substantive coverage reduction is thepackage-lock.jsonexclusion described above, which is deliberate, argued, and backstopped bycheck:bundle-budgetplus the push-to-mainarm. Every fail-closed behaviour in the grader —incompleteBudgetEvidence, its independence fromenforce, and the refusal to refresh from a measurement gap — is unchanged and asserted.git revertthe single commit on this branch. It restores the previousif:condition (ui_changed || build_changed), the inline Chromium pin steps, and the un-retried measurement loop; the new composite action, theperf_changedoutput andscripts/lighthouse-measurement-outcome.mjsare additive and become unreferenced. No migration, no data change, no provider state, and no committed baseline is modified by this PR, so the revert is complete and self-contained.Summary by CodeRabbit
New Features
Bug Fixes
Documentation