From ae53a8845d599b1f0a46b197f45facaca7e45465 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sun, 9 Aug 2026 02:00:22 +0800 Subject: [PATCH 1/3] ci(visual): stop running the visual baseline job on pull requests Owner decision, 2026-08-09: the pixel gate taxes exactly the work it exists to support. During a redesign the honest result is red on every UI pull request, and the baselines cannot be refreshed until a run has produced the new images, so that red is unavoidable rather than actionable. It was never merge-blocking -- `continue-on-error: true`, and absent from `pr-required`, whose needs list is [changes, static-pr, safety, coverage, build, container-images, ui-critical-fast, ui-critical, db-reset-verify]. But a red check nobody can act on mid-iteration is friction whether or not it gates, and it generated a steady stream of failure notifications on PR #1743 while the only correct response was "yes, that is the change". Drift is still caught. The job keeps running on pushes to main and release branches, on the weekly schedule, and on demand via workflow_dispatch. That moves the signal to where a refresh is cheap -- after the change has landed, against a run whose artifact already contains the new images -- instead of interrupting the change. One command refreshes it: npm run design-system:baselines:adopt -- --from --run-id --head --reviewed-by "" --write Nothing else changes: the job, its targets, its provenance contract and its advisory status are untouched, and no required check is added or removed. Verified: npm run check:github-actions passed; ci-cache-safety 18 passed, 11 skipped; no test pins this job to pull_request; format clean. Refs #118 --- .github/workflows/ci.yml | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8cdc3ba63..b0d152b1b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -682,7 +682,23 @@ jobs: visual-baseline: name: Visual baselines (advisory) needs: changes - if: needs.changes.outputs.ui_changed == 'true' + # Deliberately NOT on pull_request. Owner decision, 2026-08-09: a pixel gate + # that reports on every UI pull request taxes exactly the work it exists to + # support. During a redesign the honest result is red on every push, and the + # baselines cannot be refreshed until a run has produced the new images, so + # that red is unavoidable rather than actionable. It never blocked a merge + # (`continue-on-error`, and absent from `pr-required`), but a red check nobody + # can act on mid-iteration is friction whether or not it gates. + # + # Drift is still caught: this runs on pushes to main and release branches, on + # the weekly schedule, and on demand via workflow_dispatch. That puts the + # signal where a refresh is cheap — after the change has landed — instead of + # interrupting the change. Refresh with `npm run design-system:baselines:adopt` + # against that run's artifact. + # + # Do not add `pull_request` back, and do not add this job to `pr-required`, + # without the owner saying so. + if: needs.changes.outputs.ui_changed == 'true' && github.event_name != 'pull_request' continue-on-error: true runs-on: ubuntu-24.04 timeout-minutes: 45 From be0d3374d0ae00dcb77ee564dcf55014999cd16b Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sun, 9 Aug 2026 02:03:12 +0800 Subject: [PATCH 2/3] issues: record the owner decision that visual-baseline stays off pull requests --- docs/outstanding-issues.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 4b8f7dab3..32e63f0e2 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -211,7 +211,7 @@ removed after current-main verification; it is not missing recommended work. | #101 | P3 | rec | Canary-gated retrieval parallelisation candidates | **Outcome:** independent retrieval stages stop running serially, proven by a live canary pair. Candidates: metadata/memory/visual hydration triples repeated on four branches (`rag.ts:2460,2493,2521` and three more) while `rag.ts:2751-2804` already parallelises three RPCs in one `Promise.all`, so the omission is inconsistency rather than intent; the nested `await`-in-loop scope enumeration (`search-scope.ts:202,328`); typeahead results never cached (`rag.ts:2698-2711`); universal-search coalescing (`/api/search` has it, `/api/search/universal` does not). Each changes candidate assembly, truncation, or what the next keystroke returns, so each needs 36/36 retrieval plus recall 1.0 and zero per-case rr regressions. Distinct from #001 (semantic rerank). Resolved #075 and #083 are the precedents for why these are gated rather than free. **Stop:** needs the #098 harness and explicit canary approval first. | `docs/audit/latency-audit-2026-07-28.md` L2-1/L2-2/L2-8/L1-5 | 2026-07-29 | | #102 | P3 | task | Apply the additive `documents` index debt (operator) | **Outcome:** bare-column `ILIKE` and the paged status scan on `documents` are index-served on hosted. `documents_title_trgm_idx` indexes a CONCATENATED expression, so the bare-column predicates in `api/documents/route.ts:193` and `rag-candidate-sources.ts:477` (RAG path) cannot use it and fall back to scanning; `search-scope.ts:271-277` sorts per page against the single-column `documents_status_idx`. **Runbook prepared 2026-07-29 — NOT applied, item stays open:** three `CREATE INDEX CONCURRENTLY` statements authored and reviewed in `docs/operator-apply-performance-latency-remediation.md` — additive, though **the "recall is byte-identical" claim was RETRACTED on 2026-07-29 review**: `fetchDocumentTitleAliasRows` (`rag-candidate-sources.ts:482`) applies `.limit(12)` with no `ORDER BY`, so a new index can change which title-alias documents feed candidate assembly. Only the documents-list use stays ordering-safe; `(status,id)` is canary-gated too — see runbook, and making that `.limit(12)` deterministic first does **not** lift the gate — an unordered `LIMIT` has no stable selection to preserve, so imposing an order can pick a different twelve and is itself an ordering behaviour change on a retrieval surface, which AGENTS.md requires a canary pair for. Sequencing the ordering fix first is worthwhile (unordered `LIMIT` on a retrieval input is latent nondeterminism regardless) but yields two canary-gated changes, not one (PR #1377 review). **Deliberately NO migration file:** an additive-index migration without a synchronized `schema.sql` mirror and regenerated drift manifest is exactly what closed PR #1312, and the mirror cannot come first because `required_indexes` in `search_schema_health()` (`schema.sql:3178`) runs against live. **Next (operator):** **author the migration first** — `supabase/migrations/` is the source of truth and `schema.sql` only a mirror, so hand-run operator SQL never reaches staging, disaster-recovery replay, or a local `supabase db reset`, and a `required_indexes` registration would fail there (PR #1377 review); follow the `20260717170000_registry_projection_cleanup.sql` idempotent pattern. **That migration must also carry the health-function change** — `required_indexes` lives inside `search_schema_health()`, which is redefined by `create or replace function` in eleven migrations (copy `20260705180000_reconcile_search_health_indexes.sql:62`); editing `schema.sql:3177` alone moves only the mirror and leaves the indexes unmonitored on hosted (PR #1377 review). Then apply concurrently, confirm `indisvalid`, mirror both the index statements and the identical function body into `schema.sql`, run `npm run drift:manifest` (Docker), and deploy the migration LAST — in that order, in one change. Expect `check:drift` to report them as unexpected between steps 1 and 2. **Rollback is three deployed phases, not the reverse of one:** retract `required_indexes` via its own `create or replace function` migration and deploy → drop concurrently live → only then deploy the `schema.sql` removal plus an idempotent forward `drop index if exists` migration, because Supabase wraps migrations in a transaction and a plain `DROP INDEX` there takes the lock the concurrent procedure exists to avoid (PR #1377 review). | `docs/audit/latency-audit-2026-07-28.md` L2-3/L2-5; `docs/operator-apply-performance-latency-remediation.md` | 2026-07-29 | | #117 | P2 | rec | Therapy Compass catalogue payload is the mobile LCP outlier | **Outcome:** `/therapy-compass` mobile LCP lands near the other mobile routes instead of double them. **Measured 2026-07-30** by the new pre-merge Lighthouse budget: mobile LCP 5229 ms, TBT 612 ms, CLS 0.142, against 2123-2460 ms on every other mobile route and 826 ms on desktop — so it is client-side work under mobile CPU/network throttling, not server latency. **Cause before this PR:** `useTherapyData` fetched `public/therapy-compass-data/therapies-index.json` (the thin browse index; 205 records) for the home/search/pathways screens, so the download plus JSON parse sat on the critical path before content painted. **Current split:** home now fetches `public/therapy-compass-data/therapies-home.211dab554c4ec62d.json` (136,288 bytes raw), pathways use the thin browse index, and search loads the full prose corpus (#1471). 90% of the index weight is long-form clinical prose — indications 159 KB (26%), contraindicationsOrCautions 139 KB (23%), bestUsedFor 73 KB (12%), clinicalSummary 67 KB (11%), patientPopulation 59 KB (10%), targetSymptoms 48 KB (8%) — while name, slug, category, tags and setting together are 54 KB (7%). **Remaining decision for search/pathways: rendered on the card, matched by search, or neither.** `therapy-card.tsx` references five of those prose fields and the same index feeds the search screen, so stripping fields could silently change clinical display or search recall. **Next:** settle that per-field question, then either pre-truncate prose that only feeds card display, or move search matching server-side / load prose on first keystroke. **Gate:** `check:therapy-data-index` plus the therapy Playwright journeys; re-measure with `npm run verify:lighthouse`. **Stop:** do not drop a field from the catalogue payload without confirming no card renders it and no search path matches on it. Same class as #013 (route-chunk / catalogue JSON weight), different route and now measured. | session 2026-07-30 Lighthouse budget first run; PR #1404; PR #1489 | 2026-07-30 | -| #118 | P2 | task | Adopt the visual and Lighthouse baselines so the two new gates actually gate | **Outcome:** `visual-baseline` and `lighthouse-budget` stop reporting and start blocking. **Detail:** PR #1404 added both as `continue-on-error` jobs outside `pr-required`, deliberately. `tests/ui-visual-baseline.spec.ts` has no committed baselines, so all six targets (dashboard-shell, dashboard-shell-phone, search-results-band, search-results-band-phone, document-viewer, therapy-compass-home) fail with a missing-snapshot error by design; the job uploads them on every run (run 30513537912, artifact 8748062487, 31 files). `tests/__screenshots__/` still holds only README.md, re-checked 2026-08-08. `lighthouse-budget.json` ships `enforce: false` with `baseline: null`, so the grader warns rather than grades. **Owner decision 2026-08-08: adopt the baselines, advisory-only first, and promote to required only after they have held across a few runs** — the workflow comment asks for that soak explicitly. **Sequencing constraint recorded the same day:** the owner had design changes merging at the time, and pixel baselines frozen while the look is still moving go red on the next merge, which is the churn the hold exists to avoid. Trigger the ubuntu job once that merge wave has landed, not before. **Next:** (1) trigger `visual-baseline` on a UI PR, download the artifact, review the six PNGs and commit them under the platform-scoped screenshots directory that `playwright.visual.config.ts` names in its `snapshotPathTemplate` — from CI, never a developer machine, because font hinting differs between them; (2) run `npm run check:lighthouse-budget -- --update` against a known-good CI build and flip `enforce`, but not before #117 or the baseline pins a known-slow route; (3) then add each job to `pr-required` and drop `continue-on-error` in the same edit. **Also:** PR #1404 added the first rendered-effect contract for #094, but 37 of the 38 unlayered visual classes still carry exemptions in `tests/helpers/style-contracts.ts` rather than contracts; and `scripts/run-lighthouse-budget.mjs` duplicates about 50 lines of the isolated-server boot in `scripts/run-playwright.mjs`, deferred to avoid destabilising the required UI gate in the same change. **Stop:** do not make a missing baseline skip instead of fail — that is the soft-skip-green pattern `AGENTS.md` forbids. **Lighthouse half is currently failing for an unrelated reason, measured 2026-08-08 on PR #1720 (run 31248683948).** **CORRECTION 2026-08-08 (PR #1724 Codex review):** this is NOT a new CI-runner Chromium bump. Commit `4a9d81d` (#1716) already pinned the job to Playwright's Chromium 151, and the pre-#1710 baseline (`4f1bbbb^:lighthouse-budget.json`) already recorded `HeadlessChrome/151`. PR #1710 (`4f1bbbb`) then replaced those baselines with Chromium 150 data, so the advisory refuses to grade 151 measurements against a stale 150 overwrite. Prefer restoring or comparing against the already-known Chromium 151 baseline rather than a blind `--update` that silently re-accepts current numbers. Symptom unchanged: all ten route/strategy pairs report 'baseline measured by a different browser ... refresh with --update' and the job exits 1 with 'Evidence incomplete. Nothing is graded from this run.' That is a refuse-to-grade, NOT a budget breach — the measurements themselves were at or better than baseline, with mobile-root improving from LCP 3930ms to 2304ms. After #1722 the advisory is scoped to `perf_changed` (not every ui/build PR) and a `workflow_dispatch` "Refresh Lighthouse baseline" path exists — still commit only after reviewing the artifact. Refresh from CI artifact provenance only (that run uploaded `lighthouse-budget-31248683948`, artifact id 9019375154) — the script keys on the browser UA, so developer-machine numbers are rejected the same way. | session 2026-07-30; PR #1404 | 2026-07-30 | +| #118 | P2 | task | Adopt the visual and Lighthouse baselines so the two new gates actually gate | OWNER DECISION 2026-08-09 — visual-baseline is NOT to be promoted to pr-required, and no longer runs on pull_request at all (PR #1755). Supersedes step (3) of this row for the visual half: do not re-add pull_request or add the job to pr-required without the owner saying so. Reason: a pixel gate that reports on every UI PR taxes the work it exists to support — during a redesign the honest result is red on every push, and baselines cannot be refreshed until a run has produced the new images, so the red is unavoidable rather than actionable. It was never merge-blocking (continue-on-error, absent from pr-required's needs list) but the notification churn on PR #1743 was real friction. Drift is still caught on pushes to main/release, the weekly schedule, and workflow_dispatch, where a refresh costs one command: npm run design-system:baselines:adopt (added PR #1743). Accepted trade-off: a PR can now move a surface and land before anything notices; the red appears on the next main push. Narrower alternative if pre-merge coverage is ever wanted back: run it only on PRs carrying an opt-in label. The Lighthouse half of this row is unaffected and still fails repo-wide for the Chromium 150-vs-151 baseline reason recorded above. | session 2026-07-30; PR #1404 | 2026-07-30 | | #142 | P3 | task | Four loose dated docs need source and migration edits before they can be filed | **Outcome:** every dated point-in-time doc lives in `docs/audit/` or `docs/archive/` as `docs/README.md` requires, not loose at the `docs/` top level. **Detail:** PR #1436 filed the five that were docs-only moves. These four are referenced from outside `docs/`, so relocating them means editing source, tests and migration SQL comments — a different risk class than a docs tidy, and not worth bundling into one: `capacity-review.md` (`scripts/soak-test.ts`), `tenancy-defense-in-depth-review.md` (`src/lib/owner-scope.ts`, `tests/owner-scope-guard.test.ts`, two migrations, `SECURITY.md`, `.claude/agents/clinical-governance-reviewer.md`), `operator-apply-july8-batch.md` (three migrations plus `supabase/schema.sql`), `scale-readiness-review.md` (one migration). Also note `forward-codify-retrieval-rpcs-workorder.md` is indexed as a completed workorder but live is still ahead of the repo on those RPCs, so archiving it would misrepresent open operator work. **Next:** treat as low priority — the docs are correctly indexed and reachable where they are; only file them if a pass is already editing those migrations. Editing applied migration SQL is subject to `npm run check:migration-role`'s immutability pin. | PR #1436; session 2026-07-30 | 2026-07-30 | | #147 | P2 | rec | Mobile CLS breaches four routes, and it reproduces exactly offline | **Outcome:** mobile CLS is under 0.1 on every measured route, or each remaining breach has a recorded reason. **Measured 2026-07-30 with the LOCAL OFFLINE production harness** (`CHROME_PATH=/opt/pw-browsers/chromium npm run verify:lighthouse -- --keep`; Lighthouse 12.8.2, Chromium 141.0.7390.37, demo corpus, inert loopback Supabase). Mobile CLS: `/dsm` 0.363, `/documents/search` 0.220, `/therapy-compass` 0.142, `/` 0.023 — `/forms` did not measure locally (Lighthouse `NO_NAVSTART`, its own "run again" transient), but the live dispatch put it at 0.212. Desktop passes everywhere: 0.016–0.097. **The finding that matters is that these reproduce the live production dispatch EXACTLY.** Against run `30548662649` on `psychiatry.tools`, all four locally measurable mobile routes match to three decimals — 0.023 / 0.142 / 0.220 / 0.363 on both — and the desktop range matches at both endpoints, 0.016–0.097. So CLS on these routes is deterministic layout structure, not network, latency, or production data. Three consequences: (a) the CLS half of `#017` is debuggable and gate-able locally at zero provider cost; (b) `#017`'s recorded blocker "prod server hard-requires Supabase secrets" is stale — `scripts/run-lighthouse-budget.mjs` builds and serves an offline production app precisely to sidestep that, and its earlier note that dev-mode CLS looked "excellent at 0.00–0.04" was measuring DEV, which does not reproduce production; (c) re-dispatching the live workflow to re-measure CLS buys nothing. **LCP does NOT reproduce and must not be read from local runs:** local mobile LCP is 2014–2051 ms against 3639–4716 ms live, because the loopback server has no network latency. The LCP _ranking_ does survive — `/therapy-compass` is the outlier both locally (4978 ms, ~2.4x every other route) and live — which corroborates `#117` rather than replacing it. Mobile TBT, local: `/documents/search` 581 ms, `/dsm` 448, `/therapy-compass` 379, `/` 338. **Ranking by measured contribution, which is what `#017` asked for:** 1. `/dsm` CLS 0.363 (3.6x the threshold, the worst single number anywhere); 2. `/documents/search` 0.220; 3. `/forms` 0.212 (live only); 4. `/therapy-compass` 0.142, and separately the sole LCP outlier via `#117`; 5. `/` 0.023, which passes and needs nothing. **ATTRIBUTED 2026-07-30 — one cause dominates, and it is a self-inflicted round trip.** Driving Chromium directly against the same offline production build (Lighthouse mobile emulation: 412x823, DPR 1.75, 4x CPU throttle) with a `PerformanceObserver` on `layout-shift` reading `entry.sources[].node`: on `/dsm`, `/documents/search`, `/forms` and `/therapy-compass` the entire main content region moves **down 128px and straight back up 128px** within 15-60ms. Both moves score, so the round trip is pure cost with zero net movement. It is **100% of `/documents/search`** (0.110 down + 0.110 up = 0.220) and about 75% of `/dsm`. The shifting element is the `max-sm:pt-[var(--phone-overlay-chrome-h)]` wrapper around `
`, i.e. the phone overlay chrome reserve. **Mechanism, pinned by a MutationObserver timeline on the root style attribute rather than inferred:** the property goes CSS seed -> `200px` -> `72px`, and the decisive line is that the 200px is written when the header stack **already measures 72px** — `t=1552ms reserve=200px stack=72`, corrected to `72px` at `t=1612ms`. So `usePhoneOverlayChromeReserve` (`src/components/clinical-dashboard/use-phone-overlay-chrome-reserve.ts`) publishes a **stale** measurement: its `useLayoutEffect` reads `stack.offsetHeight` while the stack is still transiently 200px, and the write lands after the stack has already collapsed to 72px; its `ResizeObserver` then corrects it. The CSS seed at `globals.css:375` (`calc(max(0.5rem, var(--safe-area-top)) + var(--shell-header-h))` = 72px on a zero-inset profile) is **correct for the settled stack** — the seed is not the problem. **This corrects the mechanism recorded on `#130`** (now archived), which framed the defect as the seed under-reserving by `max(0, 0.5rem - inset)`, i.e. 0px on a notched iPhone and 8px elsewhere. Measured, the CLS driver is not an 8px seed shortfall but a 128px transient over-reserve written by the hook. Do not reopen `#130` on this; it is recorded here. **Control that makes the attribution trustworthy:** `/` never writes the property at all (no overlay stack) and is the one clean route at 0.023. **Variance, stated rather than smoothed over:** `/dsm` measured 0.363 (3 shift entries) and 0.219 (2 entries) on two runs — the round trip is the stable core and the third entry is intermittent. This harness has no network throttling, so `/forms` (0.310 local vs 0.212 live) and `/therapy-compass` (0.240 vs 0.142) run high locally; only `/dsm` 0.363, `/documents/search` 0.220 and `/` 0.023 reproduced the live dispatch exactly, and only those three should be treated as production-equivalent. **Next:** stop the hook publishing a measurement it is about to revise — defer the first publish until the stack has settled, or let the `ResizeObserver` be the only writer and trust the seed until it fires. That is a phone-chrome change, so it is governed by `docs/search-chrome-behaviour.md` and needs `npm run verify:phone-chrome` plus a before/after CLS pair from this harness. **Harness trap worth keeping:** attaching a `MutationObserver` to `document.documentElement` inside a Playwright `addInitScript` throws when the document element does not exist yet, which silently takes the CLS observer down with it and reports a uniform `CLS=0.000` across every route — a false clean bill, not a pass. Guard the attach. **Original next step, now done:** attribute the shifts to elements. Lighthouse's `layout-shift-elements` audit returned **zero items on every route** — the `cumulative-layout-shift` audit carries only `debugdata` — so attribution needs a driven Chromium session with a `PerformanceObserver` on `layout-shift`, reading `entry.sources[].node` and `entry.value`, against the same offline production build. Start with `/dsm`. **Gate — `#118` owns it, and this row supplies a constraint it was missing.** `#118` already tracks flipping `lighthouse-budget.json` to `enforce: true` and says not to do it "before `#117` or the baseline pins a known-slow route". These numbers make that concrete: pinning a baseline today would bake CLS 0.363 on `/dsm` in as the accepted value, so the CLS fixes must land first or the gate ratifies the breach. Separately, no baseline could have been committed from this run anyway — the grader correctly refused it as incomplete evidence because `/forms` produced no report, and that refusal was not overridden. **Reproduction trap:** without `CHROME_PATH`, `chrome-launcher` cannot find a browser in this container and every route fails; the harness reads `CHROME_PATH`/`PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH` (`run-lighthouse-budget.mjs:291`) but nothing sets either, even though `PLAYWRIGHT_BROWSERS_PATH` is set. Related to `#121`. Also seen: a truncated `progress` package in the npx cache broke Lighthouse startup entirely; clearing the npx cache entry fixed it. **Stop:** do not re-dispatch the live Web-Vitals workflow to re-measure CLS — it is reproducible offline for free. Do not commit a budget baseline from a run with a failed route. Do not treat local LCP numbers as production LCP. | `#017` live run `30548662649`; local `verify:lighthouse` 2026-07-30; `lighthouse-budget.json` | 2026-07-30 | | #149 | P2 | issue | `check:installed-lock-parity` only compares seven top-level packages, so transitive drift passes | **Outcome:** a session cannot run local gates against a `node_modules` that silently disagrees with the lockfile. **Evidence 2026-07-30:** the `SessionStart` hook reported "node_modules matches the lockfile, skipping install" while `brace-expansion` was installed at **1.1.16** and the lockfile pinned **1.1.18** — the CVE-2026-14257 patch. Every local gate run in that window was weaker than it appeared, including several `verify:cheap` runs reported as green during PR #1430/#1444 work. It surfaced only because `tests/installed-lock-parity.test.ts` asserts the patched behaviour directly (`maxLength` honoured); its failure was initially mistaken for a pre-existing repo defect, since it also failed on clean `main` **in the same stale container**. `npm ci --include=dev` fixed it. **CORRECTION 2026-07-30 (PR #1496 review, Codex):** an earlier version of this row said the parity check lives only in `verify:ui` and proposed adding it to `verify:cheap`. **Both were wrong** — `verify:cheap:internal` already runs `check:installed-lock-parity`, so that follow-up was a no-op, and it ran green throughout the stale window. The real gap is its **scope**: `scripts/check-installed-lock-parity.mjs` compares only seven top-level packages (`criticalInstalledPackages` = next, react, react-dom, eslint, playwright, typescript, vitest), so a stale **transitive** dependency is invisible to it. Proved by faking `node_modules/brace-expansion` back to `1.1.16` against the `1.1.18` lockfile pin: `npm run check:installed-lock-parity` **exits 0**. That also explains why the hook and the gate both reported parity while the tree was stale — neither was looking at the package that drifted. **Next:** broaden the check beyond the seven names — validate the full installed tree against the lockfile, or stamp the install with the lockfile hash and compare that — so transitive drift is caught rather than assumed absent. **Stop:** do not treat a green `check:installed-lock-parity` as proof the install is lockfile-current; today it means seven packages match. **Do not archive on the audit-worktree disposition (checked 2026-07-31):** several `comprehensive-audit-*` worktrees carry an uncommitted row archiving this as “Resolved 2026-07-31 — `check:installed-lock-parity` now validates every concrete package location in `package-lock.json`, including nested transitives”. That fix exists nowhere: not on `main`, not in any of the 42 worktrees, and not in any open PR. `main` still reads `criticalInstalledPackages = ["next", "react", "react-dom", "eslint", "playwright", "typescript", "vitest"]`. The sibling `#103` disposition from the same worktrees WAS verified true and landed in PR #1528; this one was not. **Second, worse instance measured 2026-08-08 — the gap is not only transitive version drift, it is structural completeness.** Two `npm install` runs were killed mid-extraction (a concurrent session ran `npm cache clean`; both exited **127 with an empty log**, no npm error). What survived looked complete: every package directory present at its locked version, and `check:installed-lock-parity` **passed**. But `@testing-library/dom/types/index.d.ts` was **0 bytes** and `@types/node` was **six files short**, missing `web-globals/importmeta.d.ts`. The result was **163 phantom `tsc` errors** — 135 x TS2305 'no exported member screen', plus `ImportMeta.dirname`, `Timeout` and `unref` failures — that read exactly like real code breakage, and `@rolldown/binding-win32-x64-msvc` was absent so Vitest died with `ERR_DLOPEN_FAILED`. A version comparison of any width cannot catch this; the installed files were the right version and simply incomplete. Cheap detection that worked: run the gate against pristine `origin/main` content as well — an identical error count proves the tree innocent and the install guilty. Repair that worked: `rm -rf node_modules` then ONE `npm install `, which reifies the whole tree from the lockfile without touching `package.json` or `package-lock.json`. Worth folding into this row's fix: a completeness check alongside the version comparison — but zero-byte tracked `.d.ts` plus each package's `main`/`types` entry point is NOT enough on its own. The measured miss was `@types/node/web-globals/importmeta.d.ts`, a non-entry file; `@types/node/index.d.ts` stayed present and non-empty, so that shallow check would still pass. Require a completeness method that rejects structurally incomplete installs when arbitrary package files are missing (for example a lockfile/package file inventory, an extract-integrity stamp, or an equivalent that fails closed on a deleted non-entry path), and prove it with a fixture that deletes a non-entry file while leaving entry points intact. | `scripts/check-installed-lock-parity.mjs`; `tests/installed-lock-parity.test.ts`; `package.json` | 2026-07-30 | From 4a1581ca8fc793f953aa7a55d8df4f1781630678 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 8 Aug 2026 18:39:19 +0000 Subject: [PATCH 3/3] fix(ci): keep visual-baseline off merge_group as well as PRs Allowlist push/schedule/workflow_dispatch so merge-queue UI churn does not re-run the advisory pixel gate, and document the schedule in testing.md. --- .github/workflows/ci.yml | 24 ++++++++++++++---------- docs/branch-review-ledger.md | 1 + docs/outstanding-issues.md | 2 +- docs/testing.md | 6 ++++-- tests/ci-cache-safety.test.ts | 19 +++++++++++++++++++ 5 files changed, 39 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b0d152b1b..9ca9d2080 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -682,13 +682,15 @@ jobs: visual-baseline: name: Visual baselines (advisory) needs: changes - # Deliberately NOT on pull_request. Owner decision, 2026-08-09: a pixel gate - # that reports on every UI pull request taxes exactly the work it exists to - # support. During a redesign the honest result is red on every push, and the - # baselines cannot be refreshed until a run has produced the new images, so - # that red is unavoidable rather than actionable. It never blocked a merge - # (`continue-on-error`, and absent from `pr-required`), but a red check nobody - # can act on mid-iteration is friction whether or not it gates. + # Deliberately NOT on pull_request or merge_group. Owner decision, 2026-08-09: + # a pixel gate that reports on every UI pull request taxes exactly the work it + # exists to support. During a redesign the honest result is red on every push, + # and the baselines cannot be refreshed until a run has produced the new + # images, so that red is unavoidable rather than actionable. It never blocked + # a merge (`continue-on-error`, and absent from `pr-required`), but a red check + # nobody can act on mid-iteration is friction whether or not it gates. + # `merge_group` is excluded for the same reason: queue runs are still + # pre-merge UI churn, not a post-land refresh point. # # Drift is still caught: this runs on pushes to main and release branches, on # the weekly schedule, and on demand via workflow_dispatch. That puts the @@ -696,9 +698,11 @@ jobs: # interrupting the change. Refresh with `npm run design-system:baselines:adopt` # against that run's artifact. # - # Do not add `pull_request` back, and do not add this job to `pr-required`, - # without the owner saying so. - if: needs.changes.outputs.ui_changed == 'true' && github.event_name != 'pull_request' + # Do not add `pull_request` / `merge_group` back, and do not add this job to + # `pr-required`, without the owner saying so. + if: > + needs.changes.outputs.ui_changed == 'true' && + contains(fromJSON('["push","schedule","workflow_dispatch"]'), github.event_name) continue-on-error: true runs-on: ubuntu-24.04 timeout-minutes: 45 diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index e2b180941..ae8cf2c18 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -765,3 +765,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-08-08 | claude/planning-build-intelligence-9ot0nm | 1ebc84bb288b516bb322c09cde2889e981d302a4 | AGENTS.md reasoning-effort calibration section (docs-only) | Authored and handed off as PR #1730; docs-only, pr-policy classifier returns clinicalRisk/operationalRisk/ragRanking false | prettier --check . (repo-wide, pass); docs:check-links (1665 refs resolve, pass); pr-policy classifyPullRequestFiles(AGENTS.md) | | 2026-08-08 | claude/planning-build-intelligence-9ot0nm | 2b0ad7d41d841c13515f10de7c41e449470dfa78 | pr-1730 review-and-fix | Deep review + Bugbot: no P0/P1; fixed 2 scoped P2 clarity risks (version-bump under-planning; live-state vs provider boundary). Residual: OPENAI_*_REASONING_EFFORT vocab overlap. Merge-tree clean; required CI was green pre-push. | prettier --check AGENTS.md; docs:check-links (1667); verify:pr-local (docs route pass); verify:cheap (524 files / 5607 tests pass); pr-policy classify clinical/operational/rag false; Bugbot no P0-P2 | | 2026-08-08 | dependabot/npm_and_yarn/js-yaml-4.3.1 | a79943df33e653d2a65d4db2f192ee77c22ab75a | PR #1668 unblock | late-synced main after CI green on f04a96c3; merge-tree clean (GitHub DIRTY was stale); js-yaml 4.3.1 + nanoid 3.3.18 preserved; no unresolved threads; CI re-run after push | pre-late-sync: PR required pass on f04a96c3; Production UI skipped; post-sync pending | +| 2026-08-08 | claude/ds-visual-advisory-off-prs (PR #1755) | e6d24190eb1e02c435003d429d88f2d293b14867 | heavy review-and-fix | merged origin/main; fixed Bugbot/Codex P2 merge_group exclusion via event allowlist; synced docs/testing.md + #118 note; CodeRabbit date nit dispositioned (owner +0800); threads unreplied (403) | vitest ci-cache-safety; check:github-actions; no provider-backed checks | diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 492b7cc0c..40d009108 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -211,7 +211,7 @@ removed after current-main verification; it is not missing recommended work. | #101 | P3 | rec | Canary-gated retrieval parallelisation candidates | **Outcome:** independent retrieval stages stop running serially, proven by a live canary pair. Candidates: metadata/memory/visual hydration triples repeated on four branches (`rag.ts:2460,2493,2521` and three more) while `rag.ts:2751-2804` already parallelises three RPCs in one `Promise.all`, so the omission is inconsistency rather than intent; the nested `await`-in-loop scope enumeration (`search-scope.ts:202,328`); typeahead results never cached (`rag.ts:2698-2711`); universal-search coalescing (`/api/search` has it, `/api/search/universal` does not). Each changes candidate assembly, truncation, or what the next keystroke returns, so each needs 36/36 retrieval plus recall 1.0 and zero per-case rr regressions. Distinct from #001 (semantic rerank). Resolved #075 and #083 are the precedents for why these are gated rather than free. **Stop:** needs the #098 harness and explicit canary approval first. | `docs/audit/latency-audit-2026-07-28.md` L2-1/L2-2/L2-8/L1-5 | 2026-07-29 | | #102 | P3 | task | Apply the additive `documents` index debt (operator) | **Outcome:** bare-column `ILIKE` and the paged status scan on `documents` are index-served on hosted. `documents_title_trgm_idx` indexes a CONCATENATED expression, so the bare-column predicates in `api/documents/route.ts:193` and `rag-candidate-sources.ts:477` (RAG path) cannot use it and fall back to scanning; `search-scope.ts:271-277` sorts per page against the single-column `documents_status_idx`. **Runbook prepared 2026-07-29 — NOT applied, item stays open:** three `CREATE INDEX CONCURRENTLY` statements authored and reviewed in `docs/operator-apply-performance-latency-remediation.md` — additive, though **the "recall is byte-identical" claim was RETRACTED on 2026-07-29 review**: `fetchDocumentTitleAliasRows` (`rag-candidate-sources.ts:482`) applies `.limit(12)` with no `ORDER BY`, so a new index can change which title-alias documents feed candidate assembly. Only the documents-list use stays ordering-safe; `(status,id)` is canary-gated too — see runbook, and making that `.limit(12)` deterministic first does **not** lift the gate — an unordered `LIMIT` has no stable selection to preserve, so imposing an order can pick a different twelve and is itself an ordering behaviour change on a retrieval surface, which AGENTS.md requires a canary pair for. Sequencing the ordering fix first is worthwhile (unordered `LIMIT` on a retrieval input is latent nondeterminism regardless) but yields two canary-gated changes, not one (PR #1377 review). **Deliberately NO migration file:** an additive-index migration without a synchronized `schema.sql` mirror and regenerated drift manifest is exactly what closed PR #1312, and the mirror cannot come first because `required_indexes` in `search_schema_health()` (`schema.sql:3178`) runs against live. **Next (operator):** **author the migration first** — `supabase/migrations/` is the source of truth and `schema.sql` only a mirror, so hand-run operator SQL never reaches staging, disaster-recovery replay, or a local `supabase db reset`, and a `required_indexes` registration would fail there (PR #1377 review); follow the `20260717170000_registry_projection_cleanup.sql` idempotent pattern. **That migration must also carry the health-function change** — `required_indexes` lives inside `search_schema_health()`, which is redefined by `create or replace function` in eleven migrations (copy `20260705180000_reconcile_search_health_indexes.sql:62`); editing `schema.sql:3177` alone moves only the mirror and leaves the indexes unmonitored on hosted (PR #1377 review). Then apply concurrently, confirm `indisvalid`, mirror both the index statements and the identical function body into `schema.sql`, run `npm run drift:manifest` (Docker), and deploy the migration LAST — in that order, in one change. Expect `check:drift` to report them as unexpected between steps 1 and 2. **Rollback is three deployed phases, not the reverse of one:** retract `required_indexes` via its own `create or replace function` migration and deploy → drop concurrently live → only then deploy the `schema.sql` removal plus an idempotent forward `drop index if exists` migration, because Supabase wraps migrations in a transaction and a plain `DROP INDEX` there takes the lock the concurrent procedure exists to avoid (PR #1377 review). | `docs/audit/latency-audit-2026-07-28.md` L2-3/L2-5; `docs/operator-apply-performance-latency-remediation.md` | 2026-07-29 | | #117 | P2 | rec | Therapy Compass catalogue payload is the mobile LCP outlier | **Outcome:** `/therapy-compass` mobile LCP lands near the other mobile routes instead of double them. **Measured 2026-07-30** by the new pre-merge Lighthouse budget: mobile LCP 5229 ms, TBT 612 ms, CLS 0.142, against 2123-2460 ms on every other mobile route and 826 ms on desktop — so it is client-side work under mobile CPU/network throttling, not server latency. **Cause before this PR:** `useTherapyData` fetched `public/therapy-compass-data/therapies-index.json` (the thin browse index; 205 records) for the home/search/pathways screens, so the download plus JSON parse sat on the critical path before content painted. **Current split:** home now fetches `public/therapy-compass-data/therapies-home.211dab554c4ec62d.json` (136,288 bytes raw), pathways use the thin browse index, and search loads the full prose corpus (#1471). 90% of the index weight is long-form clinical prose — indications 159 KB (26%), contraindicationsOrCautions 139 KB (23%), bestUsedFor 73 KB (12%), clinicalSummary 67 KB (11%), patientPopulation 59 KB (10%), targetSymptoms 48 KB (8%) — while name, slug, category, tags and setting together are 54 KB (7%). **Remaining decision for search/pathways: rendered on the card, matched by search, or neither.** `therapy-card.tsx` references five of those prose fields and the same index feeds the search screen, so stripping fields could silently change clinical display or search recall. **Next:** settle that per-field question, then either pre-truncate prose that only feeds card display, or move search matching server-side / load prose on first keystroke. **Gate:** `check:therapy-data-index` plus the therapy Playwright journeys; re-measure with `npm run verify:lighthouse`. **Stop:** do not drop a field from the catalogue payload without confirming no card renders it and no search path matches on it. Same class as #013 (route-chunk / catalogue JSON weight), different route and now measured. | session 2026-07-30 Lighthouse budget first run; PR #1404; PR #1489 | 2026-07-30 | -| #118 | P2 | task | Adopt the visual and Lighthouse baselines so the two new gates actually gate | OWNER DECISION 2026-08-09 — visual-baseline is NOT to be promoted to pr-required, and no longer runs on pull_request at all (PR #1755). Supersedes step (3) of this row for the visual half: do not re-add pull_request or add the job to pr-required without the owner saying so. Reason: a pixel gate that reports on every UI PR taxes the work it exists to support — during a redesign the honest result is red on every push, and baselines cannot be refreshed until a run has produced the new images, so the red is unavoidable rather than actionable. It was never merge-blocking (continue-on-error, absent from pr-required's needs list) but the notification churn on PR #1743 was real friction. Drift is still caught on pushes to main/release, the weekly schedule, and workflow_dispatch, where a refresh costs one command: npm run design-system:baselines:adopt (added PR #1743). Accepted trade-off: a PR can now move a surface and land before anything notices; the red appears on the next main push. Narrower alternative if pre-merge coverage is ever wanted back: run it only on PRs carrying an opt-in label. The Lighthouse half of this row is unaffected and still fails repo-wide for the Chromium 150-vs-151 baseline reason recorded above. | session 2026-07-30; PR #1404 | 2026-07-30 | +| #118 | P2 | task | Adopt the visual and Lighthouse baselines so the two new gates actually gate | OWNER DECISION 2026-08-09 — visual-baseline is NOT to be promoted to pr-required, and no longer runs on pull_request or merge_group (PR #1755). Supersedes step (3) of this row for the visual half: do not re-add pull_request or add the job to pr-required without the owner saying so. Reason: a pixel gate that reports on every UI PR taxes the work it exists to support — during a redesign the honest result is red on every push, and baselines cannot be refreshed until a run has produced the new images, so the red is unavoidable rather than actionable. It was never merge-blocking (continue-on-error, absent from pr-required's needs list) but the notification churn on PR #1743 was real friction. Drift is still caught on pushes to main/release, the weekly schedule, and workflow_dispatch, where a refresh costs one command: npm run design-system:baselines:adopt (added PR #1743). Accepted trade-off: a PR can now move a surface and land before anything notices; the red appears on the next main push. Narrower alternative if pre-merge coverage is ever wanted back: run it only on PRs carrying an opt-in label. The Lighthouse half of this row is unaffected and still fails repo-wide for the Chromium 150-vs-151 baseline reason recorded above. | session 2026-07-30; PR #1404 | 2026-07-30 | | #142 | P3 | task | Four loose dated docs need source and migration edits before they can be filed | **Outcome:** every dated point-in-time doc lives in `docs/audit/` or `docs/archive/` as `docs/README.md` requires, not loose at the `docs/` top level. **Detail:** PR #1436 filed the five that were docs-only moves. These four are referenced from outside `docs/`, so relocating them means editing source, tests and migration SQL comments — a different risk class than a docs tidy, and not worth bundling into one: `capacity-review.md` (`scripts/soak-test.ts`), `tenancy-defense-in-depth-review.md` (`src/lib/owner-scope.ts`, `tests/owner-scope-guard.test.ts`, two migrations, `SECURITY.md`, `.claude/agents/clinical-governance-reviewer.md`), `operator-apply-july8-batch.md` (three migrations plus `supabase/schema.sql`), `scale-readiness-review.md` (one migration). Also note `forward-codify-retrieval-rpcs-workorder.md` is indexed as a completed workorder but live is still ahead of the repo on those RPCs, so archiving it would misrepresent open operator work. **Next:** treat as low priority — the docs are correctly indexed and reachable where they are; only file them if a pass is already editing those migrations. Editing applied migration SQL is subject to `npm run check:migration-role`'s immutability pin. | PR #1436; session 2026-07-30 | 2026-07-30 | | #147 | P2 | rec | Mobile CLS breaches four routes, and it reproduces exactly offline | **Outcome:** mobile CLS is under 0.1 on every measured route, or each remaining breach has a recorded reason. **Measured 2026-07-30 with the LOCAL OFFLINE production harness** (`CHROME_PATH=/opt/pw-browsers/chromium npm run verify:lighthouse -- --keep`; Lighthouse 12.8.2, Chromium 141.0.7390.37, demo corpus, inert loopback Supabase). Mobile CLS: `/dsm` 0.363, `/documents/search` 0.220, `/therapy-compass` 0.142, `/` 0.023 — `/forms` did not measure locally (Lighthouse `NO_NAVSTART`, its own "run again" transient), but the live dispatch put it at 0.212. Desktop passes everywhere: 0.016–0.097. **The finding that matters is that these reproduce the live production dispatch EXACTLY.** Against run `30548662649` on `psychiatry.tools`, all four locally measurable mobile routes match to three decimals — 0.023 / 0.142 / 0.220 / 0.363 on both — and the desktop range matches at both endpoints, 0.016–0.097. So CLS on these routes is deterministic layout structure, not network, latency, or production data. Three consequences: (a) the CLS half of `#017` is debuggable and gate-able locally at zero provider cost; (b) `#017`'s recorded blocker "prod server hard-requires Supabase secrets" is stale — `scripts/run-lighthouse-budget.mjs` builds and serves an offline production app precisely to sidestep that, and its earlier note that dev-mode CLS looked "excellent at 0.00–0.04" was measuring DEV, which does not reproduce production; (c) re-dispatching the live workflow to re-measure CLS buys nothing. **LCP does NOT reproduce and must not be read from local runs:** local mobile LCP is 2014–2051 ms against 3639–4716 ms live, because the loopback server has no network latency. The LCP _ranking_ does survive — `/therapy-compass` is the outlier both locally (4978 ms, ~2.4x every other route) and live — which corroborates `#117` rather than replacing it. Mobile TBT, local: `/documents/search` 581 ms, `/dsm` 448, `/therapy-compass` 379, `/` 338. **Ranking by measured contribution, which is what `#017` asked for:** 1. `/dsm` CLS 0.363 (3.6x the threshold, the worst single number anywhere); 2. `/documents/search` 0.220; 3. `/forms` 0.212 (live only); 4. `/therapy-compass` 0.142, and separately the sole LCP outlier via `#117`; 5. `/` 0.023, which passes and needs nothing. **ATTRIBUTED 2026-07-30 — one cause dominates, and it is a self-inflicted round trip.** Driving Chromium directly against the same offline production build (Lighthouse mobile emulation: 412x823, DPR 1.75, 4x CPU throttle) with a `PerformanceObserver` on `layout-shift` reading `entry.sources[].node`: on `/dsm`, `/documents/search`, `/forms` and `/therapy-compass` the entire main content region moves **down 128px and straight back up 128px** within 15-60ms. Both moves score, so the round trip is pure cost with zero net movement. It is **100% of `/documents/search`** (0.110 down + 0.110 up = 0.220) and about 75% of `/dsm`. The shifting element is the `max-sm:pt-[var(--phone-overlay-chrome-h)]` wrapper around `
`, i.e. the phone overlay chrome reserve. **Mechanism, pinned by a MutationObserver timeline on the root style attribute rather than inferred:** the property goes CSS seed -> `200px` -> `72px`, and the decisive line is that the 200px is written when the header stack **already measures 72px** — `t=1552ms reserve=200px stack=72`, corrected to `72px` at `t=1612ms`. So `usePhoneOverlayChromeReserve` (`src/components/clinical-dashboard/use-phone-overlay-chrome-reserve.ts`) publishes a **stale** measurement: its `useLayoutEffect` reads `stack.offsetHeight` while the stack is still transiently 200px, and the write lands after the stack has already collapsed to 72px; its `ResizeObserver` then corrects it. The CSS seed at `globals.css:375` (`calc(max(0.5rem, var(--safe-area-top)) + var(--shell-header-h))` = 72px on a zero-inset profile) is **correct for the settled stack** — the seed is not the problem. **This corrects the mechanism recorded on `#130`** (now archived), which framed the defect as the seed under-reserving by `max(0, 0.5rem - inset)`, i.e. 0px on a notched iPhone and 8px elsewhere. Measured, the CLS driver is not an 8px seed shortfall but a 128px transient over-reserve written by the hook. Do not reopen `#130` on this; it is recorded here. **Control that makes the attribution trustworthy:** `/` never writes the property at all (no overlay stack) and is the one clean route at 0.023. **Variance, stated rather than smoothed over:** `/dsm` measured 0.363 (3 shift entries) and 0.219 (2 entries) on two runs — the round trip is the stable core and the third entry is intermittent. This harness has no network throttling, so `/forms` (0.310 local vs 0.212 live) and `/therapy-compass` (0.240 vs 0.142) run high locally; only `/dsm` 0.363, `/documents/search` 0.220 and `/` 0.023 reproduced the live dispatch exactly, and only those three should be treated as production-equivalent. **Next:** stop the hook publishing a measurement it is about to revise — defer the first publish until the stack has settled, or let the `ResizeObserver` be the only writer and trust the seed until it fires. That is a phone-chrome change, so it is governed by `docs/search-chrome-behaviour.md` and needs `npm run verify:phone-chrome` plus a before/after CLS pair from this harness. **Harness trap worth keeping:** attaching a `MutationObserver` to `document.documentElement` inside a Playwright `addInitScript` throws when the document element does not exist yet, which silently takes the CLS observer down with it and reports a uniform `CLS=0.000` across every route — a false clean bill, not a pass. Guard the attach. **Original next step, now done:** attribute the shifts to elements. Lighthouse's `layout-shift-elements` audit returned **zero items on every route** — the `cumulative-layout-shift` audit carries only `debugdata` — so attribution needs a driven Chromium session with a `PerformanceObserver` on `layout-shift`, reading `entry.sources[].node` and `entry.value`, against the same offline production build. Start with `/dsm`. **Gate — `#118` owns it, and this row supplies a constraint it was missing.** `#118` already tracks flipping `lighthouse-budget.json` to `enforce: true` and says not to do it "before `#117` or the baseline pins a known-slow route". These numbers make that concrete: pinning a baseline today would bake CLS 0.363 on `/dsm` in as the accepted value, so the CLS fixes must land first or the gate ratifies the breach. Separately, no baseline could have been committed from this run anyway — the grader correctly refused it as incomplete evidence because `/forms` produced no report, and that refusal was not overridden. **Reproduction trap:** without `CHROME_PATH`, `chrome-launcher` cannot find a browser in this container and every route fails; the harness reads `CHROME_PATH`/`PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH` (`run-lighthouse-budget.mjs:291`) but nothing sets either, even though `PLAYWRIGHT_BROWSERS_PATH` is set. Related to `#121`. Also seen: a truncated `progress` package in the npx cache broke Lighthouse startup entirely; clearing the npx cache entry fixed it. **Stop:** do not re-dispatch the live Web-Vitals workflow to re-measure CLS — it is reproducible offline for free. Do not commit a budget baseline from a run with a failed route. Do not treat local LCP numbers as production LCP. | `#017` live run `30548662649`; local `verify:lighthouse` 2026-07-30; `lighthouse-budget.json` | 2026-07-30 | | #149 | P2 | issue | `check:installed-lock-parity` only compares seven top-level packages, so transitive drift passes | **Outcome:** a session cannot run local gates against a `node_modules` that silently disagrees with the lockfile. **Evidence 2026-07-30:** the `SessionStart` hook reported "node_modules matches the lockfile, skipping install" while `brace-expansion` was installed at **1.1.16** and the lockfile pinned **1.1.18** — the CVE-2026-14257 patch. Every local gate run in that window was weaker than it appeared, including several `verify:cheap` runs reported as green during PR #1430/#1444 work. It surfaced only because `tests/installed-lock-parity.test.ts` asserts the patched behaviour directly (`maxLength` honoured); its failure was initially mistaken for a pre-existing repo defect, since it also failed on clean `main` **in the same stale container**. `npm ci --include=dev` fixed it. **CORRECTION 2026-07-30 (PR #1496 review, Codex):** an earlier version of this row said the parity check lives only in `verify:ui` and proposed adding it to `verify:cheap`. **Both were wrong** — `verify:cheap:internal` already runs `check:installed-lock-parity`, so that follow-up was a no-op, and it ran green throughout the stale window. The real gap is its **scope**: `scripts/check-installed-lock-parity.mjs` compares only seven top-level packages (`criticalInstalledPackages` = next, react, react-dom, eslint, playwright, typescript, vitest), so a stale **transitive** dependency is invisible to it. Proved by faking `node_modules/brace-expansion` back to `1.1.16` against the `1.1.18` lockfile pin: `npm run check:installed-lock-parity` **exits 0**. That also explains why the hook and the gate both reported parity while the tree was stale — neither was looking at the package that drifted. **Next:** broaden the check beyond the seven names — validate the full installed tree against the lockfile, or stamp the install with the lockfile hash and compare that — so transitive drift is caught rather than assumed absent. **Stop:** do not treat a green `check:installed-lock-parity` as proof the install is lockfile-current; today it means seven packages match. **Do not archive on the audit-worktree disposition (checked 2026-07-31):** several `comprehensive-audit-*` worktrees carry an uncommitted row archiving this as “Resolved 2026-07-31 — `check:installed-lock-parity` now validates every concrete package location in `package-lock.json`, including nested transitives”. That fix exists nowhere: not on `main`, not in any of the 42 worktrees, and not in any open PR. `main` still reads `criticalInstalledPackages = ["next", "react", "react-dom", "eslint", "playwright", "typescript", "vitest"]`. The sibling `#103` disposition from the same worktrees WAS verified true and landed in PR #1528; this one was not. **Second, worse instance measured 2026-08-08 — the gap is not only transitive version drift, it is structural completeness.** Two `npm install` runs were killed mid-extraction (a concurrent session ran `npm cache clean`; both exited **127 with an empty log**, no npm error). What survived looked complete: every package directory present at its locked version, and `check:installed-lock-parity` **passed**. But `@testing-library/dom/types/index.d.ts` was **0 bytes** and `@types/node` was **six files short**, missing `web-globals/importmeta.d.ts`. The result was **163 phantom `tsc` errors** — 135 x TS2305 'no exported member screen', plus `ImportMeta.dirname`, `Timeout` and `unref` failures — that read exactly like real code breakage, and `@rolldown/binding-win32-x64-msvc` was absent so Vitest died with `ERR_DLOPEN_FAILED`. A version comparison of any width cannot catch this; the installed files were the right version and simply incomplete. Cheap detection that worked: run the gate against pristine `origin/main` content as well — an identical error count proves the tree innocent and the install guilty. Repair that worked: `rm -rf node_modules` then ONE `npm install `, which reifies the whole tree from the lockfile without touching `package.json` or `package-lock.json`. Worth folding into this row's fix: a completeness check alongside the version comparison — but zero-byte tracked `.d.ts` plus each package's `main`/`types` entry point is NOT enough on its own. The measured miss was `@types/node/web-globals/importmeta.d.ts`, a non-entry file; `@types/node/index.d.ts` stayed present and non-empty, so that shallow check would still pass. Require a completeness method that rejects structurally incomplete installs when arbitrary package files are missing (for example a lockfile/package file inventory, an extract-integrity stamp, or an equivalent that fails closed on a deleted non-entry path), and prove it with a fixture that deletes a non-entry file while leaving entry points intact. | `scripts/check-installed-lock-parity.mjs`; `tests/installed-lock-parity.test.ts`; `package.json` | 2026-07-30 | diff --git a/docs/testing.md b/docs/testing.md index 67058524c..8c8f548a1 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -115,8 +115,10 @@ motion off with carets hidden. Baselines are committed per platform (`tests/__screenshots__/{platform}/`). **Adopt them from the CI job's artifact, not from a developer machine** — font hinting and antialiasing differ, and a laptop-generated baseline makes every CI run red. A platform with no baseline fails loudly rather -than passing silently. The CI job is `continue-on-error` until the baselines have held across a few -runs; promote it by adding it to `pr-required` and dropping that flag together. +than passing silently. The CI `visual-baseline` job is deliberately **off `pull_request` and +`merge_group`** (owner decision on PR #1755 / `#118`): it still runs on pushes to main/release, the +weekly schedule, and `workflow_dispatch`, and stays `continue-on-error` / outside `pr-required`. Do +not re-add pre-merge triggers or promote it without an explicit owner ask. ## Performance budget diff --git a/tests/ci-cache-safety.test.ts b/tests/ci-cache-safety.test.ts index 444d39912..6c92b8cea 100644 --- a/tests/ci-cache-safety.test.ts +++ b/tests/ci-cache-safety.test.ts @@ -309,6 +309,25 @@ describe.skipIf(process.platform === "win32")("PR required aggregate — cancell }); }); +describe("Visual baseline routing", () => { + /** The `visual-baseline:` block, up to the next top-level job key. */ + const visualBaselineJob = /\n visual-baseline:\n([\s\S]*?)(?=\n [a-z][\w-]*:\n)/.exec(workflow)?.[1] ?? ""; + + it("finds the visual-baseline job", () => { + expect(visualBaselineJob, "visual-baseline job not found in ci.yml").not.toBe(""); + }); + + it("stays off pull_request and merge_group; only post-land/manual events run it", () => { + // Owner decision (PR #1755 / #118): pre-merge UI churn is the wrong place for + // an unavoidably-red pixel gate. merge_group is still pre-merge. + expect(visualBaselineJob).toContain('["push","schedule","workflow_dispatch"]'); + expect(visualBaselineJob).toContain("continue-on-error: true"); + const prRequiredNeeds = /\n pr-required:\n[\s\S]*?needs:\s*\n?\s*\[([\s\S]*?)\]/.exec(workflow)?.[1] ?? ""; + expect(prRequiredNeeds, "could not read pr-required's needs list from ci.yml").not.toBe(""); + expect(prRequiredNeeds).not.toMatch(/\bvisual-baseline\b/); + }); +}); + 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] ?? "";