From e1be9dd3392e9b40ee38f793420c887167eb4c40 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 5 Sep 2026 11:02:10 -0400 Subject: [PATCH 1/6] docs(ui): design the Compass UI visual-regression gate (RIG-2154) Freeze the automated visual-regression gate for the Compass UI: built-in Playwright `toHaveScreenshot` with baselines committed in-repo and regenerated only by a pinned nix Chromium in CI, never from a developer machine. - `visual-gate` moon task hard-gates the PR from first landing - `regen-visual-baselines` `workflow_dispatch` is the only baseline writer - `maxDiffPixelRatio` 0.001 + `threshold` 0.2, per-shot floors 10-25px - 11 captures: 7 fullPage, 3 element, 1 clip - @playwright/test 1.62.1 Ledger row DL-341 lands with the record in this commit. The Open Questions section carries Matt's five load-bearing forks; per skill://design they block the merge-freeze, not the submit. Co-authored-by: Matt Wilkinson --- docs/designs/DECISIONS.md | 1 + .../compass-visual-regression-gate/design.md | 469 ++++++++++++++++++ 2 files changed, 470 insertions(+) create mode 100644 docs/designs/ui/compass-visual-regression-gate/design.md diff --git a/docs/designs/DECISIONS.md b/docs/designs/DECISIONS.md index 16fb2942..658f841a 100644 --- a/docs/designs/DECISIONS.md +++ b/docs/designs/DECISIONS.md @@ -449,3 +449,4 @@ check enforces the mechanical half. Full rationale: | DL-318 | Pre-stream/inner error and abort content surface on the session trace as a dedicated `SessionError` message (`compass.v1.SessionEvent` oneof field 10) with a `SessionErrorKind` discriminator (ERROR \| ABORTED), a failure `message`, and an optional HTTP `status` — not `SessionNotice`, not a comms `Message` block (RIG-2616) | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | | DL-322 | Error/abort emit rule: `reason=error` emits `SessionError(ERROR)` AND the existing `ERRORED` lifecycle transition (additive — board/presence/delivery key off `ERRORED`); `reason=aborted` emits `SessionError(ABORTED)` with NO lifecycle transition (abort is not a crash) and replaces the prior counted-`UnmappedEvent` staging | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | | DL-323 | The `SessionError` trace frame rides the FrameSink never-drop PRIORITY lane (not the bounded drop-oldest trace queue), via an `isSessionError` classifier extending the `frame-sink.ts` priority predicate — matching the `SessionInjection` never-drop carve-out, so surfaced failure content is as durable-on-the-spine as the lifecycle transition it reports | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | +| DL-341 | Compass UI visual regression is gated by built-in Playwright `toHaveScreenshot` with baseline PNGs committed in-repo and regenerated ONLY by a pinned nix Chromium in CI (`regen-visual-baselines` `workflow_dispatch`), never from a developer machine; a `visual-gate` moon task hard-gates the PR from first landing at `maxDiffPixelRatio` 0.001 + `threshold` 0.2 with per-shot absolute floors (10-25px), over 11 captures (7 fullPage \| 3 element \| 1 clip) on @playwright/test 1.62.1 (RIG-2154) | Active (Matt, 2026-09-06) | [visual regression gate](ui/compass-visual-regression-gate/design.md) | diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md new file mode 100644 index 00000000..a582ae16 --- /dev/null +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -0,0 +1,469 @@ +# Design: Compass automated visual-regression gate (RIG-2154) + +Status: Draft +Owner lane: compass-ux (design) → compass-ux (execution) +Refs: RIG-2154 +Governing spec: docs/designs/ui/compass-ui-fixture-boot/design.md (Decision D7) +Origin: fixture-boot D7 ruled that the visual-smoke harness graduates from a human before/after PNG-review tool to a real automated visual-regression gate, built on the deterministic fixture-boot substrate (RIG-2124, merged). + +## Problem / Intent + +The visual-smoke harness captures 11 PNGs of the core surfaces (7 full-page, +3 element close-ups, 1 clipped strip) but +asserts nothing — it is "a smoke harness: no pixel-diff gating, no +computed-style assertions" (`apps/ui/playwright.config.ts:7-8`), and CI never +runs it: the `ci` task deps are `['typecheck', 'build', 'test', 'stylelint', +'dev-smoke']` (`apps/ui/moon.yml:86`) and `dev-smoke` runs only +`bunx playwright test e2e/dev-boot.spec.ts` (`moon.yml:80`). Fixture-boot D7 +ruled the follow-up: "make the harness a real automated visual-regression gate +(`toHaveScreenshot` + a `maxDiffPixelRatio` threshold + baselines committed to +git + generated in a pinned CI environment)" +(`compass-ui-fixture-boot/design.md:510-513`). This record designs that gate. + +## Approach + +### Oracle: Playwright built-in `toHaveScreenshot`, baselines in-repo (decided) + +Built-in `expect(page).toHaveScreenshot()` with committed PNG baselines, not a +cloud service (Percy/Chromatic). This is decided here, not an open question: + +- The whole substrate was built for it. Fixture-boot's determinism knobs + (animations disabled, css-scaled raster, `deviceScaleFactor: 1`, + `reducedMotion: "reduce"`, `document.fonts.ready` awaited per shot — + `playwright.config.ts:60-62`, `visual-smoke.spec.ts:24,28-29`) exist + precisely so a raster comparison is stable; a DOM-serialization service + makes them redundant while adding an external SaaS dependency, secrets, and + cost to a harness that is offline by construction ("no daemon on :50051 and + no `VITE_COMPASS_BASE_URL` — offline by construction", + `visual-smoke.spec.ts:6-8`). +- The rendering environment is already pinned end to end: + `chromium-e2e-env.nix` "pins nixpkgs to the SAME devenv.lock revision the + dev shell and gate-tools.nix resolve, so CI drives byte-for-byte the + Chromium a Linux dev box does" (`chromium-e2e-env.nix:19-20`), and the moon + CI leg already exports `PLAYWRIGHT_CHROMIUM_PATH` from it + (`.github/workflows/ci.yml:365-367`). The cross-environment raster drift + that motivates cloud services is exactly what this pin removes. +- The API is available at the pin: `@playwright/test` is `1.62.1` + (`apps/ui/package.json:34`); `toHaveScreenshot`, `maxDiffPixels`, + `maxDiffPixelRatio`, `expect.toHaveScreenshot` config defaults, and + `snapshotPathTemplate` (since v1.28) are all long-stable in that line + (verified against upstream Playwright docs this run). + +A cloud service stays available as a later escalation if raster maintenance +cost proves high; nothing in this design forecloses it. + +Repo weight is not a concern at snapshot level — the 11 committed baselines +total ~575 KB (largest `bridge-prs.png` ~95 KB, smallest `state-dot.png` +~150 B) — but each regen rewrites all 11, so git *history* grows ~0.5 MB per +baseline-churn event; under active UI development that is plausibly tens of +MB/year of permanent history. Acceptable, and Git LFS for `e2e/__screens__/` +was weighed and rejected: it complicates the nix CI checkout and breaks the +in-diff-view image review the bot-PR baseline workflow (T4) depends on. + +### Shape: convert the generator spec in place + +`visual-smoke.spec.ts` becomes the gate spec: each capture becomes a +`toHaveScreenshot` assertion of the **same raster options it captures today**, +now asserted rather than written. The 11 captures are not uniform — the spec +takes three shapes, and each converts to its matching `toHaveScreenshot` form: + +- **7 full-page** `page.screenshot({ fullPage: true, … })` (bridge, + bridge-empty, agent, backlog, done, settings, bridge-prs) → + `await expect(page).toHaveScreenshot(".png", { fullPage: true, + animations: "disabled", scale: "css" })`. +- **3 element** `locator.screenshot(…)` (right-sidebar on `aside.right` + `visual-smoke.spec.ts:69`, state-dot on `.cx-state-dot` `:140`, bridge-card + on `.cx-card` `:204`) → `await expect(locator).toHaveScreenshot(".png", + { animations: "disabled", scale: "css" })` — no `fullPage`; the locator + bounds the raster. +- **1 clip** `page.screenshot({ clip: {…} })` (bridge-colheads, a computed + union rect over `.bridge-col-head` cells, `:180-191`) → + `await expect(page).toHaveScreenshot("bridge-colheads.png", { clip, + animations: "disabled", scale: "css" })`, keeping the bounding-box + computation untouched. + +Converting all 11 to `expect(page).toHaveScreenshot({ fullPage: true })` — as +an earlier draft of this record did — would compare full pages against the four +element/clip-sized committed baselines: a guaranteed day-one red, or worse a +regen that silently erases the close-up coverage D7 asked for. No parallel +spec: two specs capturing the same surfaces drift, and the human before/after +review workflow survives unchanged because a passing run leaves the committed +baselines as the review artifact and a failing run produces `-actual`/`-diff` +PNGs. + +`snapshotPathTemplate` is set so baselines stay at their current names: +Playwright's default template appends platform/project suffixes +(`bridge-chromium-linux.png`); a template of +`{testDir}/__screens__/{arg}{ext}` keeps the existing 11 files +(`apps/ui/e2e/__screens__/`: bridge.png, bridge-empty.png, bridge-card.png, +bridge-colheads.png, bridge-prs.png, settings.png, done.png, backlog.png, +agent.png, right-sidebar.png, state-dot.png) as the baselines with no rename. +The suffix-free template is safe because the config defines a single +`chromium` project (`playwright.config.ts:73-78`) and the gate only ever runs +on Linux against the pinned Chromium (Global Constraints); a second +project/OS would need the template revisited. + +### Threshold + +`maxDiffPixelRatio: 0.001` (0.1%) as the config-level default via +`expect: { toHaveScreenshot: { … } }`, per-shot overrides allowed. Zero +tolerance is wrong even on a pinned stack — fixture-boot's byte-identity bar +is explicitly a "same-binary, same-box determinism self-test … not a +cross-environment regression oracle" +(`compass-ui-fixture-boot/design.md:417-421`), and the gate must survive +nix-channel Chromium bumps without a fleet-wide red on every anti-aliasing +shift. 0.1% of a full-page shot is small enough to catch any real layout or +palette change while absorbing sub-pixel raster noise. Tightening later is a +one-line PR once the gate has run history. + +`maxDiffPixelRatio` is a fraction of **total image area**, and this suite +spans ~4 orders of magnitude: a full-page shot (~1280×720+, ≥900 K px) at +0.001 allows ~900 differing pixels, while `state-dot.png` (a few hundred px) +gets a budget that rounds to ~0 — effectively byte-exact, the *least* slack on +the shot most exposed to a single anti-aliasing pixel shift after a Chromium +bump. One ratio cannot serve both ends. So the 4 close-ups (right-sidebar, +state-dot, bridge-card, bridge-colheads) carry per-shot `maxDiffPixels` +overrides (start 10–25 px, each with the justifying comment Global Constraints +require) as an absolute floor, while the 7 full-page shots keep the 0.001 +ratio. The per-pixel color tolerance `threshold` (YIQ distance, Playwright +default 0.2) is left at its default **as an explicit decision** — it, not the +pixel-count knobs, is what absorbs anti-aliasing colour drift; a Chromium bump +revisits it deliberately. + +### Where it runs: a moon task inside the existing moon battery + +A new `visual-gate` moon task, added to the `ci` task's deps — not a +dedicated peer job behind the rollup. The peer-job pattern (gtk4-e2e, +dogfood-e2e) exists for legs that "realize a heavy out-of-band … closure the +bare moon gate has no business building" (`ci.yml:1124-1126`). This gate has +no such closure: the moon leg already realizes the pinned Chromium and +exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:356-367`), and +the harness's webServer is the same `vite --mode fixture` boot dev-smoke's +config already drives (`playwright.config.ts:80`). A peer job would +re-bootstrap nix + toolchain for ~a minute of Playwright. The task mirrors +`dev-smoke`'s two documented disciplines (`moon.yml:64-84`): explicit +`inputs` (dev-smoke's list plus the gate spec and `e2e/__screens__/**`) so +affected-detection schedules it, and `cache: false` because the subject is +the rendered raster resolved at run time, not a moon-hashable input. + +### Baselines: generated in CI's pinned environment, updated by a dispatch lane + +The load-bearing rule: **baselines are regenerated only in the pinned CI +environment, never committed from a dev box.** The repo already has the exact +machinery pattern: the `regen-forge-fixtures` workflow_dispatch lane runs an +operator-triggered `-update` capture and "opens a BOT PR carrying the +rewritten fixtures for human review" (`ci.yml:2263-2267`), SHA-pinned +`peter-evans/create-pull-request` included (`ci.yml:2375`). The visual gate +gets a sibling lane: dispatch → bootstrap the same toolchain + pinned +Chromium → `bunx playwright test e2e/visual-smoke.spec.ts +--update-snapshots` → bot PR with `add-paths: apps/ui/e2e/__screens__`. Matt +reviews the baseline diff as images in the PR — which is also the review +surface for intentional visual changes: land the code PR with the gate red or +with regenerated baselines from the dispatch lane, per the OQ-2 fork below. + +### Failure surfacing + +On failure Playwright writes `-actual.png`, `-expected.png`, and +`-diff.png` under `outputDir` (`e2e/.output`, +`playwright.config.ts:54`). The moon-battery job gets an +`if: failure()` `actions/upload-artifact` step (SHA-pinned, per the house +rule every action in `ci.yml` follows) scoped to `apps/ui/e2e/.output/**`, so +a red gate always carries a downloadable actual/expected/diff triplet. +Inline-in-PR diff images are OQ-3. + +### Rollout: hard gate from the first landing + +No advisory period. The determinism substrate is proven (fixture-boot T4's +byte-identity self-test), the environment is pinned byte-for-byte, the first +baselines are CI-generated in that same environment, and the 0.1% ratio +absorbs residual noise. An advisory mode needs real machinery (a +`continue-on-error` leg outside the moon battery, plus somewhere to look) and +history shows advisory gates go unread. The rollback lever if it flakes: +bump `maxDiffPixelRatio` or drop a noisy shot from the gate — each a +one-line, same-day PR. Presented as OQ-4 since the issue asks, with this as +the recommendation. + +### Coverage at v1: all 11 shots + +All 11 existing surfaces gate from day one. The set already exists as +committed, determinism-hardened baselines; curating a subset means deciding +per-surface noise levels with zero run history, and the fallback (drop a shot +that proves noisy, one-line PR) is cheaper than guessing up front. Presented +as OQ-5 with this recommendation since the issue asks. + +## Global Constraints + +- **Determinism knobs are frozen and must match the substrate exactly**: + `screenshot: "off"`, `reducedMotion: "reduce"`, `deviceScaleFactor: 1` + (`playwright.config.ts:60-62`); per-shot `animations: "disabled"`, + `scale: "css"` on every capture, plus `fullPage: true` on the 7 full-page + shots only (the 3 element and 1 clip captures are bounded by their locator / + clip rect, not `fullPage` — `visual-smoke.spec.ts:25-30,69,140,180-191,204`); + `document.fonts.ready` awaited before every capture + (`visual-smoke.spec.ts:24`); content-selector waits, never fixed sleeps + (`visual-smoke.spec.ts:8-9`). No task may loosen any of these. +- **Pinned Chromium only**: the gate runs against the Chromium realized from + `tools/toolchain/chromium-e2e-env.nix` (devenv.lock-pinned nixpkgs, + `chromium-e2e-env.nix:19-20,41`), resolved via `PLAYWRIGHT_CHROMIUM_PATH` + (`playwright.config.ts:68-70`, `ci.yml:365-367`). Single `chromium` + project, Linux only. +- **Baselines from CI only**: `apps/ui/e2e/__screens__/` PNGs are written + only by the regen dispatch lane (T4) running in the pinned environment. + A locally generated baseline is a review-rejection offense — a dev-box + Chromium raster differs and would bake local noise into the oracle + (`compass-ui-fixture-boot/design.md:419-421`). +- **API floor**: `@playwright/test 1.62.1` (`apps/ui/package.json:34`); no + version bump inside this record. Every API used (`toHaveScreenshot`, + `maxDiffPixelRatio`, `expect.toHaveScreenshot` defaults, + `snapshotPathTemplate`, `--update-snapshots`) is stable at that pin. +- **Moon task discipline**: the gate task carries explicit `inputs` and + `cache: false`, mirroring `dev-smoke`'s documented rationale + (`moon.yml:64-84`). CI actions are SHA-pinned like every action in + `ci.yml`. +- **Threshold default**: `maxDiffPixelRatio: 0.001` set once in + `playwright.config.ts` `expect.toHaveScreenshot`; the 4 close-up shots + additionally carry per-shot `maxDiffPixels` floors (10–25 px). Per-pixel + `threshold` stays at the Playwright default (0.2). Every per-shot override + carries a comment justifying it. +- House ledger conventions: this record stays `Status: Draft` until merged; + markdownlint-clean. + +## Plan + +### T1 — Gate config: `toHaveScreenshot` defaults + snapshot path + +Extend `apps/ui/playwright.config.ts` with: +`snapshotPathTemplate: "{testDir}/__screens__/{arg}{ext}"` and +`expect: { toHaveScreenshot: { maxDiffPixelRatio: 0.001 } }` — the config-level +full-page default; the per-shot `maxDiffPixels` floors for the 4 close-ups are +set at their call sites in T2, not here. `threshold` is left unset (default +0.2) as a recorded decision. No project or webServer changes — the determinism +knobs at :57-72 and the fixture-mode webServer at :79-95 are already the +substrate. + +Interfaces: + +- Modifies: `apps/ui/playwright.config.ts` (top-level `snapshotPathTemplate`, + `expect` keys on the `defineConfig` object). +- Consumes: existing `e2e/__screens__/` layout (11 PNG names). +- Test cycle: `bunx playwright test e2e/visual-smoke.spec.ts` after T2 lands + resolves baselines at the unchanged paths (T1+T2 land as one PR — T1 alone + changes nothing observable because no spec asserts yet). + +### T2 — Convert the generator spec to assertions + +In `apps/ui/e2e/visual-smoke.spec.ts`, convert each capture to its matching +`toHaveScreenshot` form (the 7/3/1 split from the Shape section), preserving +its exact current raster options: + +- **7 full-page** (bridge, bridge-empty, agent, backlog, done, settings, + bridge-prs): `page.screenshot({ path, fullPage: true, animations, scale })` + → `await expect(page).toHaveScreenshot(".png", { fullPage: true, + animations: "disabled", scale: "css" })`. +- **3 element** (right-sidebar `:69`, state-dot `:140`, bridge-card `:204`): + `.screenshot({ path, animations, scale })` → + `await expect().toHaveScreenshot(".png", { animations: + "disabled", scale: "css", maxDiffPixels: })` on the same locator — + no `fullPage`. +- **1 clip** (bridge-colheads `:189`): keep the bounding-box union computation + (`:180-188`), then `await expect(page).toHaveScreenshot("bridge-colheads.png", + { clip, animations: "disabled", scale: "css", maxDiffPixels: })`. + +The 4 close-ups carry the per-shot `maxDiffPixels` floor (T1's rationale) with +a justifying comment. Keep every navigation, selector wait, and +`document.fonts.ready` await untouched. Drop the now-unused `SCREENS` const; +import `expect` alongside `test` from `@playwright/test` +(`visual-smoke.spec.ts:1` currently imports only `test`). Update the spec +header comment: it is a gate, not a review-only generator. + +Interfaces: + +- Modifies: `apps/ui/e2e/visual-smoke.spec.ts` (11 capture blocks, imports, + header comment). +- Consumes: T1's config keys; existing baselines as the initial oracle + (superseded by T4's CI regen before the gate wires into CI — see T5 + ordering). +- Produces: a spec that exits non-zero on visual drift, writing + `-actual`/`-expected`/`-diff` PNGs under `e2e/.output` on failure. +- Test cycle: local run passes against freshly `--update-snapshots`-generated + local baselines (NOT committed); a deliberate CSS perturbation reds the + matching shot; revert restores green. + +### T3 — Moon task + battery artifact upload + +Add to `apps/ui/moon.yml` a `visual-gate` task: +`command: 'bunx playwright test e2e/visual-smoke.spec.ts'`, +`deps: ['install']`, `options: { cache: false }`, and explicit `inputs` = +`dev-smoke`'s list (`moon.yml:82`) with `e2e/dev-boot.spec.ts` swapped for +`e2e/visual-smoke.spec.ts` plus `e2e/__screens__/**/*` and `src/**/*.css` +(already covered by `src/**/*`). Add `'visual-gate'` to the `ci` deps list +(`moon.yml:86`). In `.github/workflows/ci.yml`, add to the moon-battery job +an `if: failure()` SHA-pinned `actions/upload-artifact` step uploading +`apps/ui/e2e/.output/**` (name: `visual-gate-diffs`, short retention) with +`if-no-files-found: ignore` — the step fires on *any* bun-leg failure (a red +typecheck, not just a visual diff), and without that knob a no-diff failure +emits a spurious missing-artifact warning. A red gate still always ships the +diff triplet. + +Interfaces: + +- Modifies: `apps/ui/moon.yml` (new task + `ci` deps), `.github/workflows/ci.yml` + (one upload step in the moon-battery job). +- Consumes: `PLAYWRIGHT_CHROMIUM_PATH` already exported in that job + (`ci.yml:365-367`). +- Test cycle: a scratch PR with a deliberate visual change reds `ci` via + `visual-gate` and carries the `visual-gate-diffs` artifact; a no-op PR + stays green. Verify affected-detection schedules the task on a + baseline-only change. + +### T4 — Baseline regen dispatch lane + +Add a `regen-visual-baselines` workflow_dispatch job to +`.github/workflows/ci.yml`, modeled on `regen-forge-fixtures` +(`ci.yml:2260-2385`) but with two corrections the sibling-of-forge framing +hides: + +- **Discriminator input (must-fix):** `regen-forge-fixtures` gates on + `workflow_dispatch && inputs.pr == ''` (`ci.yml:2272-2274`). A second lane + with the *same* gate means every bare `ci.yml` dispatch fires BOTH — a + visual regen would also launch the 90-minute live forge capture and open a + spurious forge bot PR. Add a `regen` choice dispatch input (`forge` | + `visual`, **default `forge`**) and extend each lane's `if:` with + `&& inputs.regen == ''`. Default `forge` preserves the existing + bare-dispatch behavior of the forge lane; the visual lane fires only on an + explicit `regen: visual`. +- **JS install (must-fix):** `regen-forge-fixtures`' payload is `go test` and + installs no JS deps; this lane's payload `bunx playwright test` needs + `apps/ui` node_modules (`@playwright/test`, vite), so it adds a + `bun install` / `moon :install` step the "same two-phase bootstrap" phrase + does not cover. + +Otherwise as forge: widened `contents: write` + `pull-requests: write`, the +two-phase toolchain bootstrap (`ci.yml:2304-2328`) plus the pinned-Chromium +realization step (`ci.yml:365-367`'s pattern), then +`bunx playwright test e2e/visual-smoke.spec.ts --update-snapshots` under +`apps/ui`, then SHA-pinned `peter-evans/create-pull-request` with +`add-paths: apps/ui/e2e/__screens__`. No secrets needed (offline fixture +boot). + +Interfaces: + +- Modifies: `.github/workflows/ci.yml` (one new job + a `regen` dispatch input; + extends `regen-forge-fixtures`' `if:` with `&& inputs.regen == 'forge'` — + the only edit this record makes to an existing lane; still does not join the + rollup's `needs`, same as regen-forge-fixtures per `ci.yml:2271`). +- Produces: a bot PR carrying the regenerated 11 baselines for Matt's image + review. +- Test cycle: dispatch the lane on a branch; verify the bot PR opens with + only `__screens__` changes and the images render in the PR diff view. + +### T5 — First CI-generated baselines + cutover ordering + +Sequencing task, not a code task. Order: land T1+T2+T4 with the gate NOT yet +in `ci` deps (T3's moon.yml edit split out); dispatch T4's lane to produce +the first pinned-environment baselines; merge that bot PR (replacing the 11 +dev-box PNGs currently committed); **burn in before flipping the gate** — +re-dispatch T4's lane 5–10 times and diff the resulting bot-PR baselines +against each other: on a byte-for-byte pinned Chromium they should be +identical, and this converts the "should be deterministic" claim into +measured cross-run data at zero extra machinery (the only cross-run evidence +today is same-box, `compass-ui-fixture-boot/design.md:417-421`); then land T3 +wiring the gate into `ci`. This guarantees the gate never runs against +dev-box baselines — the first red would otherwise be a false environment-skew +red on an unrelated PR. **Race window:** between merging the baseline bot PR +and landing T3, a visually-material UI PR could merge and make T3's first run +red on main — land T3 immediately after the baseline merge, and re-dispatch +T4 if any UI-touching PR slipped in between. During that window a local +`bunx playwright test e2e/visual-smoke.spec.ts` reds against the stale +dev-box baselines (cosmetic — do not "fix" it). Also update the fixture-boot +record's D7 cross-reference and the harness docs/comments that describe it as +review-only (`playwright.config.ts:4-8` header). + +Interfaces: + +- Modifies: PR sequencing only, plus `playwright.config.ts:4-8` comment and + a one-line D7 follow-up note in + `docs/designs/ui/compass-ui-fixture-boot/design.md` (Status footnote, not a + content change). +- Test cycle: the 5–10-run burn-in shows identical baselines; after cutover, + two consecutive CI runs on main are green; a deliberate-perturbation scratch + PR reds. + +## Tasks + +- [ ] T1 — `snapshotPathTemplate` + `expect.toHaveScreenshot` defaults in + `playwright.config.ts` (lands with T2) +- [ ] T2 — convert `visual-smoke.spec.ts` captures to `toHaveScreenshot` + assertions +- [ ] T3 — `visual-gate` moon task + `ci` dep + failure-artifact upload + (lands LAST, after T5's baseline cutover) +- [ ] T4 — `regen-visual-baselines` workflow_dispatch lane → bot PR +- [ ] T5 — dispatch T4, merge first CI-generated baselines, then land T3; + update harness comments + fixture-boot D7 cross-ref + +## Open Questions + +Load-bearing (need Matt's ruling before the impl issues file): + +1. **OQ-1 — Threshold start value.** `maxDiffPixelRatio` at 0.001 (0.1%, the + issue's suggestion — absorbs anti-aliasing noise, catches layout/palette + changes), vs 0.0005 (tighter; more sensitive to Chromium-bump raster + drift), vs 0.002 (looser; risks missing a small real regression like a + 1px border change on a large full-page shot). Note the ratio is + area-scaled, so the 4 close-up shots additionally take per-shot + `maxDiffPixels` floors (10–25 px) rather than the ratio, and per-pixel + `threshold` stays at the default 0.2 (see Threshold section). + **Recommendation: 0.001** for the full-page default + the close-up floors, + revisit with run history. +2. **OQ-2 — Intentional-visual-change workflow.** When a PR intentionally + changes a surface: + (a) author lands the PR with the gate red, then dispatches the regen lane + and merges the bot PR (gate red on main briefly); + (b) author dispatches the regen lane **on the feature branch** (the native + workflow_dispatch run-from-branch selector) — `peter-evans/create-pull-request` + defaults its `base` to the checked-out branch, so the bot PR targets that + branch with the regenerated baselines and no extra input is needed (the + earlier "`ref` dispatch input" idea was unnecessary machinery); the feature + PR then lands green with its own baselines; + (c) allow a documented local `--update-snapshots` + commit, breaking the + baselines-from-CI-only rule; + (d) the lane pushes the regenerated baselines as a direct commit to the PR + branch (`contents: write`, no bot PR) — fewer steps, but removes the human + in-diff image review the bot PR gives; + (e) author iterates locally with `--update-snapshots`, then a required CI + regen replaces those files before merge (merge-queue-style). + **Two under-weighted costs on (b):** (i) per intentional change the author + pays dispatch → full toolchain+Playwright bootstrap → merge bot PR into own + branch → re-run gate — a multi-step, multi-minute loop on what may be the + *most common* change shape in an actively-developed UI, not the exception; + (ii) **jj hazard** — merging a bot PR into a jj-managed feature branch puts + a commit on the GitHub bookmark the local jj working copy lacks, and the + mandated `sync-before-submit` rebase (or any bookmark rewrite) before the + next `jj-vine submit` can silently drop the bot's baseline commit, + resurrecting the red gate with no obvious cause. Any CI-writes-to-your-branch + scheme (b/d) collides with jj bookmark rewriting. + **Recommendation: (b)** — keeps main always green and the CI-only rule + intact — but Matt should rule with the jj collision and the per-PR loop + cost on the table. +3. **OQ-3 — Diff visibility for adjudication.** (a) CI artifact zip only + (T3's design — Matt downloads the actual/expected/diff triplet); (b) + additionally a bot PR-comment embedding the diff images (needs an image + host or committing diffs to a scratch branch — more machinery, images + inline); (c) a Playwright HTML report artifact instead of raw PNGs + (single browsable file, still a download). **Recommendation: (a)** at + v1 — zero new machinery; escalate to (b) only if the download step + proves to be real friction. Deferrable: the artifact upload lands + either way. +4. **OQ-4 — Hard gate vs advisory start.** (a) hard gate from first landing + (Approach's case: pinned env + CI-generated baselines + 0.1% ratio leave + little to stabilize, and the rollback is a one-line threshold bump); (b) + a 2-week advisory period (`continue-on-error` leg) collecting flake data + first. **Recommendation: (a)**, with the T5 ordering as the safety + mechanism. +5. **OQ-5 — v1 coverage.** (a) all 11 shots (Approach's case: baselines + exist, dropping a noisy shot later is one line); (b) a curated core + subset (bridge, settings, agent, state-dot) to minimize initial noise + surface. **Recommendation: (a)**. + +Non-load-bearing / deferrable: OQ-3's escalation path; whether the regen +lane later folds into a label-triggered automation (out of scope here). From 0fa9b6bb3561f423168d43de935a1ff2bbfacdff Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 05:06:19 -0400 Subject: [PATCH 2/6] docs(ui): correct the visual-gate threshold mechanism and stale citations (RIG-2154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the record's central threshold mechanism inverted against Playwright 1.62.1's actual comparator. The record prescribed a config-level `maxDiffPixelRatio` of 0.001 plus per-shot `maxDiffPixels` of 10-25px described as an "absolute floor". Playwright resolves the two pixel-count knobs with `Math.min`, not max (`playwright-core/lib/coreBundle.js:7556-7562`), and a config-level ratio is never stripped from a per-call merge — `maxDiffPixelRatio` is absent from `NonConfigProperties` (`playwright/lib/matchers/expect.js:12391-12398`, merged at `:12419-12423`). So a per-shot `maxDiffPixels` could only ever tighten tolerance. Measured against the committed baselines, `state-dot.png` (9x10, area 90) resolved to an effective tolerance of 0px — byte-exact, precisely the outcome the section existed to prevent — while `bridge-colheads` and `right-sidebar` were tightened rather than widened. The widening knob is now a per-shot `maxDiffPixelRatio`, which does replace the config default, computed as `max(base, floor / area)`: `state-dot` 0.1111, `bridge-card` 0.00117, and explicitly no override for `bridge-colheads` or `right-sidebar` (their area-scaled budgets already exceed the intended slack). Global Constraints, T1, T2 and OQ-1 are restated to match; OQ-1 stays open. Also: - Re-resolve every `.github/workflows/ci.yml` citation. The file has grown to 2434 lines and the offsets were stale by +2 early and +48 in the regen region, so cited ranges landed in the wrong job. Each citation now carries a durable job-name/quoted-string anchor alongside the number. - Narrow DL-341 to what is actually ruled. The row froze tolerance values, the hard-gate rollout, and capture coverage while OQ-1/OQ-4/OQ-5 still await Matt's ruling; it now states the mechanism decisions only and names those three as open. Co-authored-by: Matt Wilkinson --- docs/designs/DECISIONS.md | 2 +- .../compass-visual-regression-gate/design.md | 131 ++++++++++++------ 2 files changed, 91 insertions(+), 42 deletions(-) diff --git a/docs/designs/DECISIONS.md b/docs/designs/DECISIONS.md index 658f841a..18def791 100644 --- a/docs/designs/DECISIONS.md +++ b/docs/designs/DECISIONS.md @@ -449,4 +449,4 @@ check enforces the mechanical half. Full rationale: | DL-318 | Pre-stream/inner error and abort content surface on the session trace as a dedicated `SessionError` message (`compass.v1.SessionEvent` oneof field 10) with a `SessionErrorKind` discriminator (ERROR \| ABORTED), a failure `message`, and an optional HTTP `status` — not `SessionNotice`, not a comms `Message` block (RIG-2616) | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | | DL-322 | Error/abort emit rule: `reason=error` emits `SessionError(ERROR)` AND the existing `ERRORED` lifecycle transition (additive — board/presence/delivery key off `ERRORED`); `reason=aborted` emits `SessionError(ABORTED)` with NO lifecycle transition (abort is not a crash) and replaces the prior counted-`UnmappedEvent` staging | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | | DL-323 | The `SessionError` trace frame rides the FrameSink never-drop PRIORITY lane (not the bounded drop-oldest trace queue), via an `isSessionError` classifier extending the `frame-sink.ts` priority predicate — matching the `SessionInjection` never-drop carve-out, so surfaced failure content is as durable-on-the-spine as the lifecycle transition it reports | Active (Matt, 2026-09-02) | [error/abort surfacing](agent/compass-agent-error-abort-surfacing/design.md) | -| DL-341 | Compass UI visual regression is gated by built-in Playwright `toHaveScreenshot` with baseline PNGs committed in-repo and regenerated ONLY by a pinned nix Chromium in CI (`regen-visual-baselines` `workflow_dispatch`), never from a developer machine; a `visual-gate` moon task hard-gates the PR from first landing at `maxDiffPixelRatio` 0.001 + `threshold` 0.2 with per-shot absolute floors (10-25px), over 11 captures (7 fullPage \| 3 element \| 1 clip) on @playwright/test 1.62.1 (RIG-2154) | Active (Matt, 2026-09-06) | [visual regression gate](ui/compass-visual-regression-gate/design.md) | +| DL-341 | Compass UI visual regression is gated by built-in Playwright `toHaveScreenshot` (not a hosted visual-diff service), with baseline PNGs committed in-repo and regenerated ONLY by a pinned nix Chromium in CI via a `regen-visual-baselines` `workflow_dispatch` lane, never from a developer machine; wired as a `visual-gate` moon task on @playwright/test 1.62.1. Tolerance values, hard-gate-vs-advisory rollout, and capture coverage are NOT ruled here \| they are the record's open questions (OQ-1/OQ-4/OQ-5) awaiting Matt (RIG-2154) | Active (Matt, 2026-09-06) | [visual regression gate](ui/compass-visual-regression-gate/design.md) | diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md index a582ae16..dc5c1b90 100644 --- a/docs/designs/ui/compass-visual-regression-gate/design.md +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -41,7 +41,7 @@ cloud service (Percy/Chromatic). This is decided here, not an open question: dev shell and gate-tools.nix resolve, so CI drives byte-for-byte the Chromium a Linux dev box does" (`chromium-e2e-env.nix:19-20`), and the moon CI leg already exports `PLAYWRIGHT_CHROMIUM_PATH` from it - (`.github/workflows/ci.yml:365-367`). The cross-environment raster drift + (`.github/workflows/ci.yml:369` — `PLAYWRIGHT_CHROMIUM_PATH` export in `moon-battery`). The cross-environment raster drift that motivates cloud services is exactly what this pin removes. - The API is available at the pin: `@playwright/test` is `1.62.1` (`apps/ui/package.json:34`); `toHaveScreenshot`, `maxDiffPixels`, @@ -118,26 +118,62 @@ one-line PR once the gate has run history. `maxDiffPixelRatio` is a fraction of **total image area**, and this suite spans ~4 orders of magnitude: a full-page shot (~1280×720+, ≥900 K px) at -0.001 allows ~900 differing pixels, while `state-dot.png` (a few hundred px) -gets a budget that rounds to ~0 — effectively byte-exact, the *least* slack on -the shot most exposed to a single anti-aliasing pixel shift after a Chromium -bump. One ratio cannot serve both ends. So the 4 close-ups (right-sidebar, -state-dot, bridge-card, bridge-colheads) carry per-shot `maxDiffPixels` -overrides (start 10–25 px, each with the justifying comment Global Constraints -require) as an absolute floor, while the 7 full-page shots keep the 0.001 -ratio. The per-pixel color tolerance `threshold` (YIQ distance, Playwright -default 0.2) is left at its default **as an explicit decision** — it, not the -pixel-count knobs, is what absorbs anti-aliasing colour drift; a Chromium bump -revisits it deliberately. +0.001 allows ~900 differing pixels, while `state-dot.png` (9×10 = 90 px) gets +a budget of 0.09 px — effectively byte-exact, the *least* slack on the shot +most exposed to a single anti-aliasing pixel shift after a Chromium bump. One +ratio cannot serve both ends, so the 4 close-ups (right-sidebar, state-dot, +bridge-card, bridge-colheads) need a per-shot widening. + +**The widening knob is a per-shot `maxDiffPixelRatio`, not `maxDiffPixels`.** +At the 1.62.1 pin the two pixel-count knobs resolve with `Math.min`, not max: +the explicit `maxDiffPixels` and the area-scaled ratio budget are computed +independently and, when both are present, the *smaller* wins +(`playwright-core/lib/coreBundle.js:7556-7562`). A config-level +`maxDiffPixelRatio` is always present, because the per-call merge is +`{...filteredConfigOptions, ...this.options}` +(`playwright/lib/matchers/expect.js:12419-12423`) and `NonConfigProperties` +(`expect.js:12391-12398`) strips only `clip`, `fullPage`, `mask`, `maskColor`, +`omitBackground`, `signal` — `maxDiffPixelRatio` is absent from that list, so +it is never stripped. A per-shot `maxDiffPixels` alongside a config-level +ratio can therefore only ever *tighten* tolerance, and "floor" is the wrong +word for it. Measured against the committed baseline dimensions, an earlier +draft's 10–25 px "floors" resolve to: + +| Shot | Baseline | Area (px) | Ratio budget @0.001 | Stated floor | Effective tolerance | +| --- | --- | --- | --- | --- | --- | +| `state-dot.png` | 9×10 | 90 | 0.09 px | 10 px | **0 px** — byte-exact; floor fully inert | +| `bridge-card.png` | 189×113 | 21357 | 21.36 px | 25 px | 21 px — floor inert | +| `bridge-colheads.png` | 855×41 | 35055 | 35.05 px | 25 px | 25 px — floor *tightens* by ~10 px | +| `right-sidebar.png` | 400×650 | 260000 | 260 px | 25 px | 25 px — floor *tightens* ~10× | + +That inverts exactly what this section set out to prevent: the shot needing +the most slack would get none at all. + +The correct override is a per-shot `maxDiffPixelRatio`, which *does* replace +the config default (it is a plain key in that spread, not a `Math.min` +sibling), computed as `max(base, floor / area)` against whichever base ratio +OQ-1 settles on: + +- `state-dot.png` — `max(0.001, 10/90)` = **0.1111**. +- `bridge-card.png` — `max(0.001, 25/21357)` = **0.00117**. +- `bridge-colheads.png` and `right-sidebar.png` — **no override**. At the base + ratio their area-scaled budgets (35 px and 260 px) already exceed the + intended ~25 px of slack, so any per-shot value could only tighten them. An + executor must not add one. + +The 7 full-page shots likewise take no override. The per-pixel color tolerance +`threshold` (YIQ distance, Playwright default 0.2) is left at its default +**as an explicit decision** — it, not the pixel-count knobs, is what absorbs +anti-aliasing colour drift; a Chromium bump revisits it deliberately. ### Where it runs: a moon task inside the existing moon battery A new `visual-gate` moon task, added to the `ci` task's deps — not a dedicated peer job behind the rollup. The peer-job pattern (gtk4-e2e, dogfood-e2e) exists for legs that "realize a heavy out-of-band … closure the -bare moon gate has no business building" (`ci.yml:1124-1126`). This gate has +bare moon gate has no business building" (`ci.yml:1128`, `moon-battery` peer-job rationale: "no business building"). This gate has no such closure: the moon leg already realizes the pinned Chromium and -exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:356-367`), and +exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:369`, `moon-battery`: `PLAYWRIGHT_CHROMIUM_PATH` export), and the harness's webServer is the same `vite --mode fixture` boot dev-smoke's config already drives (`playwright.config.ts:80`). A peer job would re-bootstrap nix + toolchain for ~a minute of Playwright. The task mirrors @@ -152,8 +188,8 @@ The load-bearing rule: **baselines are regenerated only in the pinned CI environment, never committed from a dev box.** The repo already has the exact machinery pattern: the `regen-forge-fixtures` workflow_dispatch lane runs an operator-triggered `-update` capture and "opens a BOT PR carrying the -rewritten fixtures for human review" (`ci.yml:2263-2267`), SHA-pinned -`peter-evans/create-pull-request` included (`ci.yml:2375`). The visual gate +rewritten fixtures for human review" (`ci.yml:2314`, `regen-forge-fixtures`: "BOT PR carrying the rewritten fixtures"), SHA-pinned +`peter-evans/create-pull-request` included (`ci.yml:2423`, `regen-forge-fixtures`: `peter-evans/create-pull-request`). The visual gate gets a sibling lane: dispatch → bootstrap the same toolchain + pinned Chromium → `bunx playwright test e2e/visual-smoke.spec.ts --update-snapshots` → bot PR with `add-paths: apps/ui/e2e/__screens__`. Matt @@ -205,7 +241,7 @@ as OQ-5 with this recommendation since the issue asks. - **Pinned Chromium only**: the gate runs against the Chromium realized from `tools/toolchain/chromium-e2e-env.nix` (devenv.lock-pinned nixpkgs, `chromium-e2e-env.nix:19-20,41`), resolved via `PLAYWRIGHT_CHROMIUM_PATH` - (`playwright.config.ts:68-70`, `ci.yml:365-367`). Single `chromium` + (`playwright.config.ts:68-70`, `ci.yml:369` — `moon-battery`'s `PLAYWRIGHT_CHROMIUM_PATH` export). Single `chromium` project, Linux only. - **Baselines from CI only**: `apps/ui/e2e/__screens__/` PNGs are written only by the regen dispatch lane (T4) running in the pinned environment. @@ -220,11 +256,15 @@ as OQ-5 with this recommendation since the issue asks. `cache: false`, mirroring `dev-smoke`'s documented rationale (`moon.yml:64-84`). CI actions are SHA-pinned like every action in `ci.yml`. -- **Threshold default**: `maxDiffPixelRatio: 0.001` set once in - `playwright.config.ts` `expect.toHaveScreenshot`; the 4 close-up shots - additionally carry per-shot `maxDiffPixels` floors (10–25 px). Per-pixel +- **Threshold default**: `maxDiffPixelRatio` set once in + `playwright.config.ts` `expect.toHaveScreenshot` (base ratio per OQ-1); + `state-dot` and `bridge-card` additionally carry a per-shot + `maxDiffPixelRatio` override of `max(base, floor / area)` — the only knob + that can widen a small shot's budget, since a per-shot `maxDiffPixels` + would resolve to `Math.min` against the config ratio and could only tighten + it (see Threshold). No shot carries a `maxDiffPixels`. Per-pixel `threshold` stays at the Playwright default (0.2). Every per-shot override - carries a comment justifying it. + carries a comment justifying it. No task may loosen any of these. - House ledger conventions: this record stays `Status: Draft` until merged; markdownlint-clean. @@ -234,9 +274,10 @@ as OQ-5 with this recommendation since the issue asks. Extend `apps/ui/playwright.config.ts` with: `snapshotPathTemplate: "{testDir}/__screens__/{arg}{ext}"` and -`expect: { toHaveScreenshot: { maxDiffPixelRatio: 0.001 } }` — the config-level -full-page default; the per-shot `maxDiffPixels` floors for the 4 close-ups are -set at their call sites in T2, not here. `threshold` is left unset (default +`expect: { toHaveScreenshot: { maxDiffPixelRatio: } }` — the config-level +default (base ratio per OQ-1, recommendation 0.001); the two per-shot +`maxDiffPixelRatio` overrides are set at their call sites in T2, not here. +`threshold` is left unset (default 0.2) as a recorded decision. No project or webServer changes — the determinism knobs at :57-72 and the fixture-mode webServer at :79-95 are already the substrate. @@ -263,14 +304,19 @@ its exact current raster options: - **3 element** (right-sidebar `:69`, state-dot `:140`, bridge-card `:204`): `.screenshot({ path, animations, scale })` → `await expect().toHaveScreenshot(".png", { animations: - "disabled", scale: "css", maxDiffPixels: })` on the same locator — - no `fullPage`. + "disabled", scale: "css" })` on the same locator — no `fullPage` — plus + `maxDiffPixelRatio: 0.1111` on state-dot and `0.00117` on bridge-card. + right-sidebar takes **no** override. - **1 clip** (bridge-colheads `:189`): keep the bounding-box union computation (`:180-188`), then `await expect(page).toHaveScreenshot("bridge-colheads.png", - { clip, animations: "disabled", scale: "css", maxDiffPixels: })`. - -The 4 close-ups carry the per-shot `maxDiffPixels` floor (T1's rationale) with -a justifying comment. Keep every navigation, selector wait, and + { clip, animations: "disabled", scale: "css" })` — **no** per-shot override. + +Only `state-dot` and `bridge-card` carry a per-shot `maxDiffPixelRatio` +(Threshold's `max(base, floor / area)`, recomputed if OQ-1 moves the base), +each with a justifying comment; `bridge-colheads` and `right-sidebar` already +have area-scaled budgets above the intended slack, so adding an override there +would only tighten them. No shot gets a `maxDiffPixels`. Keep every navigation, +selector wait, and `document.fonts.ready` await untouched. Drop the now-unused `SCREENS` const; import `expect` alongside `test` from `@playwright/test` (`visual-smoke.spec.ts:1` currently imports only `test`). Update the spec @@ -310,7 +356,7 @@ Interfaces: - Modifies: `apps/ui/moon.yml` (new task + `ci` deps), `.github/workflows/ci.yml` (one upload step in the moon-battery job). - Consumes: `PLAYWRIGHT_CHROMIUM_PATH` already exported in that job - (`ci.yml:365-367`). + (`ci.yml:369`, `moon-battery`'s `PLAYWRIGHT_CHROMIUM_PATH` export). - Test cycle: a scratch PR with a deliberate visual change reds `ci` via `visual-gate` and carries the `visual-gate-diffs` artifact; a no-op PR stays green. Verify affected-detection schedules the task on a @@ -320,11 +366,11 @@ Interfaces: Add a `regen-visual-baselines` workflow_dispatch job to `.github/workflows/ci.yml`, modeled on `regen-forge-fixtures` -(`ci.yml:2260-2385`) but with two corrections the sibling-of-forge framing +(`ci.yml:2308-2434`, `regen-forge-fixtures` job span) but with two corrections the sibling-of-forge framing hides: - **Discriminator input (must-fix):** `regen-forge-fixtures` gates on - `workflow_dispatch && inputs.pr == ''` (`ci.yml:2272-2274`). A second lane + `workflow_dispatch && inputs.pr == ''` (`ci.yml:2322`, `regen-forge-fixtures`: `github.event.inputs.pr == ''`). A second lane with the *same* gate means every bare `ci.yml` dispatch fires BOTH — a visual regen would also launch the 90-minute live forge capture and open a spurious forge bot PR. Add a `regen` choice dispatch input (`forge` | @@ -339,8 +385,8 @@ hides: does not cover. Otherwise as forge: widened `contents: write` + `pull-requests: write`, the -two-phase toolchain bootstrap (`ci.yml:2304-2328`) plus the pinned-Chromium -realization step (`ci.yml:365-367`'s pattern), then +two-phase toolchain bootstrap (`ci.yml:2352-2376`, `regen-forge-fixtures`: "Phase one"/"Phase two") plus the pinned-Chromium +realization step (`ci.yml:369`'s `moon-battery` `PLAYWRIGHT_CHROMIUM_PATH` export pattern), then `bunx playwright test e2e/visual-smoke.spec.ts --update-snapshots` under `apps/ui`, then SHA-pinned `peter-evans/create-pull-request` with `add-paths: apps/ui/e2e/__screens__`. No secrets needed (offline fixture @@ -351,7 +397,7 @@ Interfaces: - Modifies: `.github/workflows/ci.yml` (one new job + a `regen` dispatch input; extends `regen-forge-fixtures`' `if:` with `&& inputs.regen == 'forge'` — the only edit this record makes to an existing lane; still does not join the - rollup's `needs`, same as regen-forge-fixtures per `ci.yml:2271`). + rollup's `needs`, same as regen-forge-fixtures (`ci.yml:2319`, `regen-forge-fixtures`: "rollup check above ... does not `needs:` this job"). - Produces: a bot PR carrying the regenerated 11 baselines for Matt's image review. - Test cycle: dispatch the lane on a branch; verify the bot PR opens with @@ -410,11 +456,14 @@ Load-bearing (need Matt's ruling before the impl issues file): changes), vs 0.0005 (tighter; more sensitive to Chromium-bump raster drift), vs 0.002 (looser; risks missing a small real regression like a 1px border change on a large full-page shot). Note the ratio is - area-scaled, so the 4 close-up shots additionally take per-shot - `maxDiffPixels` floors (10–25 px) rather than the ratio, and per-pixel - `threshold` stays at the default 0.2 (see Threshold section). - **Recommendation: 0.001** for the full-page default + the close-up floors, - revisit with run history. + area-scaled, so `state-dot` and `bridge-card` take a per-shot + `maxDiffPixelRatio` override derived from whichever base Matt rules for — + `max(base, floor / area)`, so at 0.001 they are 0.1111 and 0.00117, and a + different base shifts them (a base above `floor / area` removes the + override entirely) — while per-pixel `threshold` stays at the default 0.2 + (see Threshold section). + **Recommendation: 0.001** for the base ratio, with the two derived per-shot + overrides, revisit with run history. 2. **OQ-2 — Intentional-visual-change workflow.** When a PR intentionally changes a surface: (a) author lands the PR with the gate red, then dispatches the regen lane From 652259fdc9df80b9c98624c3d5dd321988fa1c07 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 05:10:48 -0400 Subject: [PATCH 3/6] docs(ui): clear the visual-gate record's non-blocking review findings (RIG-2154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The peer-job rationale named `dogfood-e2e`, which is not a job in ci.yml. The dogfood suite runs inside the `e2e` job, split out for a privileged container rather than closure weight; only a cache-key prefix carries that string. Replaced with `microvm` (`ci.yml:624`), a genuine closure-weight peer job. The argument it supports is unchanged. - T2's test cycle told the executor to run a local `--update-snapshots` and parenthesized the baselines as "NOT committed", but that command silently rewrites all 11 tracked baseline PNGs and nothing ignores `__screens__/` (it must stay tracked — the baselines are the oracle). Added an explicit `jj restore apps/ui/e2e/__screens__` after the verification run, and stated in Global Constraints that the dirtying is by design, so the CI-only rule is enforced by review rather than tooling. - The `snapshotPathTemplate` justification understated the default it avoids: the default relocates every baseline into a `-snapshots/` subdirectory as well as appending project/suffix segments, so the chosen template prevents a directory move, not just a rename. - T5's ordering parenthetical implied T3 lands split across two steps while the Tasks checklist treats it as atomic. Restated to match. Co-authored-by: Matt Wilkinson --- .../compass-visual-regression-gate/design.md | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md index dc5c1b90..7ffabf5d 100644 --- a/docs/designs/ui/compass-visual-regression-gate/design.md +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -92,12 +92,14 @@ baselines as the review artifact and a failing run produces `-actual`/`-diff` PNGs. `snapshotPathTemplate` is set so baselines stay at their current names: -Playwright's default template appends platform/project suffixes -(`bridge-chromium-linux.png`); a template of -`{testDir}/__screens__/{arg}{ext}` keeps the existing 11 files +Playwright's default template relocates each baseline into a +`-snapshots/` subdirectory (`apps/ui/e2e/visual-smoke.spec.ts-snapshots/`) +and appends platform/project suffixes (`bridge-chromium-linux.png`); a template +of `{testDir}/__screens__/{arg}{ext}` keeps the existing 11 files (`apps/ui/e2e/__screens__/`: bridge.png, bridge-empty.png, bridge-card.png, bridge-colheads.png, bridge-prs.png, settings.png, done.png, backlog.png, -agent.png, right-sidebar.png, state-dot.png) as the baselines with no rename. +agent.png, right-sidebar.png, state-dot.png) as the baselines with no directory +move or rename. The suffix-free template is safe because the config defines a single `chromium` project (`playwright.config.ts:73-78`) and the gate only ever runs on Linux against the pinned Chromium (Global Constraints); a second @@ -169,9 +171,10 @@ anti-aliasing colour drift; a Chromium bump revisits it deliberately. ### Where it runs: a moon task inside the existing moon battery A new `visual-gate` moon task, added to the `ci` task's deps — not a -dedicated peer job behind the rollup. The peer-job pattern (gtk4-e2e, -dogfood-e2e) exists for legs that "realize a heavy out-of-band … closure the -bare moon gate has no business building" (`ci.yml:1128`, `moon-battery` peer-job rationale: "no business building"). This gate has +dedicated peer job behind the rollup. The peer-job pattern (gtk4-e2e, microvm) +exists for legs that "realize a heavy out-of-band … closure the bare moon gate +has no business building" (`ci.yml:1128`, `moon-battery` peer-job rationale: "no +business building"; `ci.yml:624`, `microvm` job). This gate has no such closure: the moon leg already realizes the pinned Chromium and exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:369`, `moon-battery`: `PLAYWRIGHT_CHROMIUM_PATH` export), and the harness's webServer is the same `vite --mode fixture` boot dev-smoke's @@ -245,8 +248,10 @@ as OQ-5 with this recommendation since the issue asks. project, Linux only. - **Baselines from CI only**: `apps/ui/e2e/__screens__/` PNGs are written only by the regen dispatch lane (T4) running in the pinned environment. - A locally generated baseline is a review-rejection offense — a dev-box - Chromium raster differs and would bake local noise into the oracle + A local `--update-snapshots` run dirties these tracked files by design, so + this CI-only baseline rule is enforced by review, not by tooling. A locally + generated baseline is a review-rejection offense — a dev-box Chromium raster + differs and would bake local noise into the oracle (`compass-ui-fixture-boot/design.md:419-421`). - **API floor**: `@playwright/test 1.62.1` (`apps/ui/package.json:34`); no version bump inside this record. Every API used (`toHaveScreenshot`, @@ -332,8 +337,10 @@ Interfaces: - Produces: a spec that exits non-zero on visual drift, writing `-actual`/`-expected`/`-diff` PNGs under `e2e/.output` on failure. - Test cycle: local run passes against freshly `--update-snapshots`-generated - local baselines (NOT committed); a deliberate CSS perturbation reds the - matching shot; revert restores green. + local baselines (NOT committed); restore the tracked baselines immediately + after verification with `jj restore apps/ui/e2e/__screens__` (`git checkout --` + is the git equivalent); a deliberate CSS perturbation reds the matching shot; + revert restores green. ### T3 — Moon task + battery artifact upload @@ -406,9 +413,9 @@ Interfaces: ### T5 — First CI-generated baselines + cutover ordering Sequencing task, not a code task. Order: land T1+T2+T4 with the gate NOT yet -in `ci` deps (T3's moon.yml edit split out); dispatch T4's lane to produce -the first pinned-environment baselines; merge that bot PR (replacing the 11 -dev-box PNGs currently committed); **burn in before flipping the gate** — +in `ci` deps (T3 lands in full only after the cutover); dispatch T4's lane to +produce the first pinned-environment baselines; merge that bot PR (replacing the +11 dev-box PNGs currently committed); **burn in before flipping the gate** — re-dispatch T4's lane 5–10 times and diff the resulting bot-PR baselines against each other: on a byte-for-byte pinned Chromium they should be identical, and this converts the "should be deterministic" claim into From 42ff6621f2e82edec2756c1c794056c9155a836f Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 05:32:48 -0400 Subject: [PATCH 4/6] docs(ui): correct the per-shot ratio literals and job anchors (RIG-2154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review pass confirmed the threshold mechanism fix is materially correct — verified against real Playwright 1.62.1 — but found the fix reintroduced its own defect one pixel deep, plus three anchor and consistency errors. - The derived overrides were written as truncated decimals. The comparator tests `count > area * ratio` with no rounding, so `0.1111` on state-dot yields a budget of 9.999px and tolerates 9 pixels, one short of the intended 10; `0.00117` on bridge-card yields 24.988px and tolerates 24, not 25. Both are now prescribed as exact fractions (`10/90`, `25/21357`), which give budgets of exactly 10.0 and 25.0. Any decimal written for readability is the rounded-up form. The Threshold section now states the no-rounding constraint so it is self-enforcing, and T2 prescribes the call-site form as a derived expression rather than a transcribed literal. - The Threshold section's opening still said all four close-ups need a per-shot widening, contradicting its own conclusion — and the explicit "an executor must not add one" — thirty lines later. bridge-colheads and right-sidebar already carry 35px and 260px of area-scaled budget. The opening now names only the two shots that take an override. - The durable anchors added in the previous commit named a `moon-battery` job, which does not exist: the job is `moon` (ci.yml:236) and `Moon battery` is a step within it (ci.yml:378). All nine anchors corrected; the line numbers were already right and are unchanged. The "no business building" quote at ci.yml:1128 is reattributed to `gtk4-e2e` (ci.yml:1122), whose own rationale it is. - T3's failure-artifact step specified a fixed artifact name against a four-leg matrix job, which is a hard error under upload-artifact v4+ when a second leg fails, and dropped the `matrix.run == 'true'` conjunct every step in that job carries. Now `visual-gate-diffs-${{ matrix.group }}` gated on `if: failure() && matrix.run == 'true'`, with a note that actions/upload-artifact is new to the file and needs a fresh SHA pin. Co-authored-by: Matt Wilkinson --- .../compass-visual-regression-gate/design.md | 86 +++++++++++++------ 1 file changed, 62 insertions(+), 24 deletions(-) diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md index 7ffabf5d..df16ffd8 100644 --- a/docs/designs/ui/compass-visual-regression-gate/design.md +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -41,7 +41,8 @@ cloud service (Percy/Chromatic). This is decided here, not an open question: dev shell and gate-tools.nix resolve, so CI drives byte-for-byte the Chromium a Linux dev box does" (`chromium-e2e-env.nix:19-20`), and the moon CI leg already exports `PLAYWRIGHT_CHROMIUM_PATH` from it - (`.github/workflows/ci.yml:369` — `PLAYWRIGHT_CHROMIUM_PATH` export in `moon-battery`). The cross-environment raster drift + (`.github/workflows/ci.yml:369` — `PLAYWRIGHT_CHROMIUM_PATH` export in the + `moon` job, `ci.yml:236`). The cross-environment raster drift that motivates cloud services is exactly what this pin removes. - The API is available at the pin: `@playwright/test` is `1.62.1` (`apps/ui/package.json:34`); `toHaveScreenshot`, `maxDiffPixels`, @@ -123,8 +124,11 @@ spans ~4 orders of magnitude: a full-page shot (~1280×720+, ≥900 K px) at 0.001 allows ~900 differing pixels, while `state-dot.png` (9×10 = 90 px) gets a budget of 0.09 px — effectively byte-exact, the *least* slack on the shot most exposed to a single anti-aliasing pixel shift after a Chromium bump. One -ratio cannot serve both ends, so the 4 close-ups (right-sidebar, state-dot, -bridge-card, bridge-colheads) need a per-shot widening. +ratio cannot serve both ends, so the 2 *smallest* shots (`state-dot`, +`bridge-card`) need a per-shot widening. The two larger close-ups +(`bridge-colheads`, `right-sidebar`) do not: at the base ratio their +area-scaled budgets are already 35 px and 260 px, comfortably above the +intended slack. **The widening knob is a per-shot `maxDiffPixelRatio`, not `maxDiffPixels`.** At the 1.62.1 pin the two pixel-count knobs resolve with `Math.min`, not max: @@ -156,13 +160,24 @@ the config default (it is a plain key in that spread, not a `Math.min` sibling), computed as `max(base, floor / area)` against whichever base ratio OQ-1 settles on: -- `state-dot.png` — `max(0.001, 10/90)` = **0.1111**. -- `bridge-card.png` — `max(0.001, 25/21357)` = **0.00117**. +- `state-dot.png` — `max(0.001, 10/90)`, prescribed as the **exact fraction + `10/90`** (budget exactly 10.0 px, tolerates 10 px). If a decimal is written + instead it must be the rounded-UP **0.1112**, never the truncated form. +- `bridge-card.png` — `max(0.001, 25/21357)`, prescribed as the **exact + fraction `25/21357`** (budget exactly 25.0 px, tolerates 25 px). As a decimal + it must be the rounded-UP **0.001171**. - `bridge-colheads.png` and `right-sidebar.png` — **no override**. At the base ratio their area-scaled budgets (35 px and 260 px) already exceed the intended ~25 px of slack, so any per-shot value could only tighten them. An executor must not add one. +The comparator tests `count > area * ratio` with no rounding +(`playwright-core/lib/coreBundle.js:7556-7562`), so a per-shot ratio must be +expressed as the exact fraction — or rounded UP — never truncated: a truncated +decimal silently tightens the budget by a pixel. So the call sites in T2 write +the ratio as a derived expression (`maxDiffPixelRatio: Math.max(BASE, 10/90)`), +computed rather than transcribed, which cannot drift when Matt rules OQ-1. + The 7 full-page shots likewise take no override. The per-pixel color tolerance `threshold` (YIQ distance, Playwright default 0.2) is left at its default **as an explicit decision** — it, not the pixel-count knobs, is what absorbs @@ -173,10 +188,12 @@ anti-aliasing colour drift; a Chromium bump revisits it deliberately. A new `visual-gate` moon task, added to the `ci` task's deps — not a dedicated peer job behind the rollup. The peer-job pattern (gtk4-e2e, microvm) exists for legs that "realize a heavy out-of-band … closure the bare moon gate -has no business building" (`ci.yml:1128`, `moon-battery` peer-job rationale: "no -business building"; `ci.yml:624`, `microvm` job). This gate has +has no business building" (`ci.yml:1128` — the `gtk4-e2e` job's own rationale +for being peeled out of the moon gate; that job starts at `ci.yml:1122`; +`ci.yml:624`, `microvm` job). This gate has no such closure: the moon leg already realizes the pinned Chromium and -exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:369`, `moon-battery`: `PLAYWRIGHT_CHROMIUM_PATH` export), and +exports `PLAYWRIGHT_CHROMIUM_PATH` for `dev-smoke` (`ci.yml:369`, inside the +`moon` job — `ci.yml:236`, whose `Moon battery` step is at `ci.yml:378`), and the harness's webServer is the same `vite --mode fixture` boot dev-smoke's config already drives (`playwright.config.ts:80`). A peer job would re-bootstrap nix + toolchain for ~a minute of Playwright. The task mirrors @@ -204,8 +221,8 @@ with regenerated baselines from the dispatch lane, per the OQ-2 fork below. On failure Playwright writes `-actual.png`, `-expected.png`, and `-diff.png` under `outputDir` (`e2e/.output`, -`playwright.config.ts:54`). The moon-battery job gets an -`if: failure()` `actions/upload-artifact` step (SHA-pinned, per the house +`playwright.config.ts:54`). The `moon` job (`ci.yml:236`) gets an +`if: failure() && matrix.run == 'true'` `actions/upload-artifact` step (SHA-pinned, per the house rule every action in `ci.yml` follows) scoped to `apps/ui/e2e/.output/**`, so a red gate always carries a downloadable actual/expected/diff triplet. Inline-in-PR diff images are OQ-3. @@ -244,7 +261,8 @@ as OQ-5 with this recommendation since the issue asks. - **Pinned Chromium only**: the gate runs against the Chromium realized from `tools/toolchain/chromium-e2e-env.nix` (devenv.lock-pinned nixpkgs, `chromium-e2e-env.nix:19-20,41`), resolved via `PLAYWRIGHT_CHROMIUM_PATH` - (`playwright.config.ts:68-70`, `ci.yml:369` — `moon-battery`'s `PLAYWRIGHT_CHROMIUM_PATH` export). Single `chromium` + (`playwright.config.ts:68-70`, `ci.yml:369` — the `PLAYWRIGHT_CHROMIUM_PATH` + export in the `moon` job, `ci.yml:236`). Single `chromium` project, Linux only. - **Baselines from CI only**: `apps/ui/e2e/__screens__/` PNGs are written only by the regen dispatch lane (T4) running in the pinned environment. @@ -310,7 +328,9 @@ its exact current raster options: `.screenshot({ path, animations, scale })` → `await expect().toHaveScreenshot(".png", { animations: "disabled", scale: "css" })` on the same locator — no `fullPage` — plus - `maxDiffPixelRatio: 0.1111` on state-dot and `0.00117` on bridge-card. + `maxDiffPixelRatio: Math.max(BASE, 10/90)` on state-dot and + `Math.max(BASE, 25/21357)` on bridge-card — exact fractions, per Threshold + (as rounded-up decimals, 0.1112 and 0.001171). right-sidebar takes **no** override. - **1 clip** (bridge-colheads `:189`): keep the bounding-box union computation (`:180-188`), then `await expect(page).toHaveScreenshot("bridge-colheads.png", @@ -350,22 +370,38 @@ Add to `apps/ui/moon.yml` a `visual-gate` task: `dev-smoke`'s list (`moon.yml:82`) with `e2e/dev-boot.spec.ts` swapped for `e2e/visual-smoke.spec.ts` plus `e2e/__screens__/**/*` and `src/**/*.css` (already covered by `src/**/*`). Add `'visual-gate'` to the `ci` deps list -(`moon.yml:86`). In `.github/workflows/ci.yml`, add to the moon-battery job -an `if: failure()` SHA-pinned `actions/upload-artifact` step uploading -`apps/ui/e2e/.output/**` (name: `visual-gate-diffs`, short retention) with -`if-no-files-found: ignore` — the step fires on *any* bun-leg failure (a red -typecheck, not just a visual diff), and without that knob a no-diff failure -emits a spurious missing-artifact warning. A red gate still always ships the -diff triplet. +(`moon.yml:86`). In `.github/workflows/ci.yml`, add to the `moon` job +(`ci.yml:236`) an `actions/upload-artifact` step uploading +`apps/ui/e2e/.output/**` (short retention) with `if-no-files-found: ignore` — +the step fires on *any* bun-leg failure (a red typecheck, not just a visual +diff), and without that knob a no-diff failure emits a spurious +missing-artifact warning. A red gate still always ships the diff triplet. + +Two matrix-shaped requirements on that step, both easy to get wrong: + +- **The artifact name must carry the leg:** + `visual-gate-diffs-${{ matrix.group }}`, not a fixed `visual-gate-diffs`. + The `moon` job is a matrix over the run-time-discovered concern groups + (`ci.yml:246-249`), and under `actions/upload-artifact` v4+ two concurrent + legs uploading the same artifact name is a hard error, not a merge. +- **The gate must keep the matrix conjunct:** + `if: failure() && matrix.run == 'true'`. Every step in that job is gated on + `matrix.run == 'true'` because an unaffected group is a placeholder leg that + spins up and no-ops (`ci.yml:275-279`); a bare `if: failure()` drops that + invariant and would fire on placeholder legs. + +`actions/upload-artifact` appears nowhere in `ci.yml` today, so its 40-hex SHA +pin must be added fresh — every action currently in the file is SHA-pinned and +this one is no exception. Interfaces: - Modifies: `apps/ui/moon.yml` (new task + `ci` deps), `.github/workflows/ci.yml` - (one upload step in the moon-battery job). + (one upload step in the `moon` job — `ci.yml:236`). - Consumes: `PLAYWRIGHT_CHROMIUM_PATH` already exported in that job - (`ci.yml:369`, `moon-battery`'s `PLAYWRIGHT_CHROMIUM_PATH` export). + (`ci.yml:369`, the `moon` job's `PLAYWRIGHT_CHROMIUM_PATH` export). - Test cycle: a scratch PR with a deliberate visual change reds `ci` via - `visual-gate` and carries the `visual-gate-diffs` artifact; a no-op PR + `visual-gate` and carries the `visual-gate-diffs-` artifact; a no-op PR stays green. Verify affected-detection schedules the task on a baseline-only change. @@ -393,7 +429,8 @@ hides: Otherwise as forge: widened `contents: write` + `pull-requests: write`, the two-phase toolchain bootstrap (`ci.yml:2352-2376`, `regen-forge-fixtures`: "Phase one"/"Phase two") plus the pinned-Chromium -realization step (`ci.yml:369`'s `moon-battery` `PLAYWRIGHT_CHROMIUM_PATH` export pattern), then +realization step (the `moon` job's `PLAYWRIGHT_CHROMIUM_PATH` export pattern at +`ci.yml:369`), then `bunx playwright test e2e/visual-smoke.spec.ts --update-snapshots` under `apps/ui`, then SHA-pinned `peter-evans/create-pull-request` with `add-paths: apps/ui/e2e/__screens__`. No secrets needed (offline fixture @@ -465,7 +502,8 @@ Load-bearing (need Matt's ruling before the impl issues file): 1px border change on a large full-page shot). Note the ratio is area-scaled, so `state-dot` and `bridge-card` take a per-shot `maxDiffPixelRatio` override derived from whichever base Matt rules for — - `max(base, floor / area)`, so at 0.001 they are 0.1111 and 0.00117, and a + `max(base, floor / area)` expressed as an exact fraction, so at 0.001 they + are `10/90` and `25/21357` (budgets of exactly 10.0 px and 25.0 px), and a different base shifts them (a base above `floor / area` removes the override entirely) — while per-pixel `threshold` stays at the default 0.2 (see Threshold section). From 40f6768aa2280ef68ae2224b179a0b460c0b5e6c Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 06:00:51 -0400 Subject: [PATCH 5/6] docs(ui): make the per-shot ratio executable and its scope conditional (RIG-2154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review pass confirmed the previous round's four fixes correct against a calibrated real-comparator harness, and found that round's own fix had made the call-site prescription unexecutable. - The derived-expression form `Math.max(BASE, 10/90)` referenced a symbol defined nowhere: `BASE` appeared three times, all uses, no binding, and `playwright.config.ts` exports only the config object, so nothing was importable. An executor transcribing it got a ReferenceError — the previous text was wrong-but-executable, this was neither. The wrapper was also unreachable in principle: a per-shot `maxDiffPixelRatio` replaces the config base rather than combining with it, so the base is not in scope at the call site. Both sites now prescribe the resolved literal fractions (`10/90`, `25/21357`), with `max(base, floor / area)` kept in prose as the derivation the reader performs. - The claim that bridge-colheads and right-sidebar need no override was stated unconditionally, and as an absolute "an executor must not add one", but the 35px/260px budgets behind it hold only at base 0.001 — which OQ-1 has not settled. At 0.0005 bridge-colheads falls to 17.53px, below the 25px of intended slack, so the record's own rule yields an override there. Both statements are now conditional on the ruling, right-sidebar is noted safe at every candidate base (130px even at 0.0005), and the base-specific figures carry an `@0.001` tag. The Global Constraints bullet and T2's tail restated the same flat claim and are scoped to match. No base is chosen — OQ-1 remains open. - The comparator citation for the unrounded `count > budget` test pointed at the budget computation two lines above it; corrected to `:7557,7564`. The `Math.min` citation it was borrowed from is unchanged. - Noted that the exact-fraction form needs a per-area check: float division can land `area * (floor / area)` a hair under the floor, in which case the rounded-up decimal is required. Both current cells are clean. Co-authored-by: Matt Wilkinson --- .../compass-visual-regression-gate/design.md | 87 ++++++++++++------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md index df16ffd8..ee1a5617 100644 --- a/docs/designs/ui/compass-visual-regression-gate/design.md +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -125,10 +125,11 @@ spans ~4 orders of magnitude: a full-page shot (~1280×720+, ≥900 K px) at a budget of 0.09 px — effectively byte-exact, the *least* slack on the shot most exposed to a single anti-aliasing pixel shift after a Chromium bump. One ratio cannot serve both ends, so the 2 *smallest* shots (`state-dot`, -`bridge-card`) need a per-shot widening. The two larger close-ups -(`bridge-colheads`, `right-sidebar`) do not: at the base ratio their -area-scaled budgets are already 35 px and 260 px, comfortably above the -intended slack. +`bridge-card`) need a per-shot widening. At the recommended base the two +larger close-ups (`bridge-colheads`, `right-sidebar`) do not: their +area-scaled budgets @0.001 are already 35 px and 260 px, comfortably above +the intended slack. Whether `bridge-colheads` still qualifies is +base-dependent — see the per-shot list below. **The widening knob is a per-shot `maxDiffPixelRatio`, not `maxDiffPixels`.** At the 1.62.1 pin the two pixel-count knobs resolve with `Math.min`, not max: @@ -166,17 +167,36 @@ OQ-1 settles on: - `bridge-card.png` — `max(0.001, 25/21357)`, prescribed as the **exact fraction `25/21357`** (budget exactly 25.0 px, tolerates 25 px). As a decimal it must be the rounded-UP **0.001171**. -- `bridge-colheads.png` and `right-sidebar.png` — **no override**. At the base - ratio their area-scaled budgets (35 px and 260 px) already exceed the - intended ~25 px of slack, so any per-shot value could only tighten them. An - executor must not add one. +- `bridge-colheads.png` and `right-sidebar.png` — **no override at the + recommended base 0.001**, where their area-scaled budgets (35 px and + 260 px @0.001) already exceed the intended ~25 px of slack. + `right-sidebar` (400×650 = 260000 px) needs no override at any base OQ-1 + names — even at 0.0005 its budget is 130 px. `bridge-colheads` + (855×41 = 35055 px) is base-dependent: at 0.0005 its budget falls to + 17.53 px, *below* the 25 px of intended slack, so under that ruling it + takes `25/35055` by the same `max(base, floor / area)` rule as the other + two shots. An executor adds an override here only where that rule yields + one for the base Matt rules for. The comparator tests `count > area * ratio` with no rounding -(`playwright-core/lib/coreBundle.js:7556-7562`), so a per-shot ratio must be -expressed as the exact fraction — or rounded UP — never truncated: a truncated -decimal silently tightens the budget by a pixel. So the call sites in T2 write -the ratio as a derived expression (`maxDiffPixelRatio: Math.max(BASE, 10/90)`), -computed rather than transcribed, which cannot drift when Matt rules OQ-1. +(`playwright-core/lib/coreBundle.js:7557,7564`), so a per-shot ratio must be +expressed as the exact fraction — or rounded UP — never truncated: a +truncated decimal silently tightens the budget by a pixel. The exact +fraction is not unconditionally safe either: float division can land +`area * (floor / area)` a hair *under* the floor (it does for ~74,000 of the +first million areas), so each fraction must be verified to land at or above +the floor for its own area, and where it lands short the rounded-UP decimal +is the required form. Both current cells are clean — `90 * (10/90)` is +exactly 10, and `21357 * (25/21357)` is 25.000000000000004. + +The call sites in T2 therefore write the *resolved* literal fraction rather +than a derived expression: a per-shot `maxDiffPixelRatio` replaces the +config base instead of combining with it, so the base ratio is not in scope +at the call site and `max(base, floor / area)` is a derivation the reader +performs here, not an expression the spec evaluates. At the recommended +0.001 it resolves to `10/90` and `25/21357`; if OQ-1 moves the base, +re-resolve both against that rule — a base at or above `floor / area` +removes the override entirely. The 7 full-page shots likewise take no override. The per-pixel color tolerance `threshold` (YIQ distance, Playwright default 0.2) is left at its default @@ -222,9 +242,10 @@ with regenerated baselines from the dispatch lane, per the OQ-2 fork below. On failure Playwright writes `-actual.png`, `-expected.png`, and `-diff.png` under `outputDir` (`e2e/.output`, `playwright.config.ts:54`). The `moon` job (`ci.yml:236`) gets an -`if: failure() && matrix.run == 'true'` `actions/upload-artifact` step (SHA-pinned, per the house -rule every action in `ci.yml` follows) scoped to `apps/ui/e2e/.output/**`, so -a red gate always carries a downloadable actual/expected/diff triplet. +`if: failure() && matrix.run == 'true'` `actions/upload-artifact` step +(SHA-pinned, per the house rule every action in `ci.yml` follows) scoped to +`apps/ui/e2e/.output/**`, so a red gate always carries a downloadable +actual/expected/diff triplet. Inline-in-PR diff images are OQ-3. ### Rollout: hard gate from the first landing @@ -281,8 +302,10 @@ as OQ-5 with this recommendation since the issue asks. `ci.yml`. - **Threshold default**: `maxDiffPixelRatio` set once in `playwright.config.ts` `expect.toHaveScreenshot` (base ratio per OQ-1); - `state-dot` and `bridge-card` additionally carry a per-shot - `maxDiffPixelRatio` override of `max(base, floor / area)` — the only knob + at the recommended base 0.001 `state-dot` and `bridge-card` additionally + carry a per-shot `maxDiffPixelRatio` of `max(base, floor / area)` + resolved to a literal fraction (the set is base-dependent — see + Threshold) — the only knob that can widen a small shot's budget, since a per-shot `maxDiffPixels` would resolve to `Math.min` against the config ratio and could only tighten it (see Threshold). No shot carries a `maxDiffPixels`. Per-pixel @@ -328,21 +351,23 @@ its exact current raster options: `.screenshot({ path, animations, scale })` → `await expect().toHaveScreenshot(".png", { animations: "disabled", scale: "css" })` on the same locator — no `fullPage` — plus - `maxDiffPixelRatio: Math.max(BASE, 10/90)` on state-dot and - `Math.max(BASE, 25/21357)` on bridge-card — exact fractions, per Threshold - (as rounded-up decimals, 0.1112 and 0.001171). - right-sidebar takes **no** override. + `maxDiffPixelRatio: 10/90` on state-dot and `maxDiffPixelRatio: 25/21357` + on bridge-card — exact fractions, per Threshold (as rounded-up decimals, + 0.1112 and 0.001171). These are the resolved values of Threshold's + `max(base, floor / area)` at the recommended base 0.001; re-resolve both + if OQ-1 moves the base. right-sidebar takes **no** override. - **1 clip** (bridge-colheads `:189`): keep the bounding-box union computation (`:180-188`), then `await expect(page).toHaveScreenshot("bridge-colheads.png", - { clip, animations: "disabled", scale: "css" })` — **no** per-shot override. - -Only `state-dot` and `bridge-card` carry a per-shot `maxDiffPixelRatio` -(Threshold's `max(base, floor / area)`, recomputed if OQ-1 moves the base), -each with a justifying comment; `bridge-colheads` and `right-sidebar` already -have area-scaled budgets above the intended slack, so adding an override there -would only tighten them. No shot gets a `maxDiffPixels`. Keep every navigation, -selector wait, and -`document.fonts.ready` await untouched. Drop the now-unused `SCREENS` const; + { clip, animations: "disabled", scale: "css" })` — **no** per-shot + override at the recommended base 0.001. + +At the recommended base 0.001 only `state-dot` and `bridge-card` carry a +per-shot `maxDiffPixelRatio`, each with a justifying comment: `right-sidebar` +has an area-scaled budget above the intended slack at every base OQ-1 names, +and `bridge-colheads` has one at 0.001 — under a 0.0005 ruling it gains a +`25/35055` override too (see Threshold). No shot gets a `maxDiffPixels`. +Keep every navigation, selector wait, and `document.fonts.ready` await +untouched. Drop the now-unused `SCREENS` const; import `expect` alongside `test` from `@playwright/test` (`visual-smoke.spec.ts:1` currently imports only `test`). Update the spec header comment: it is a gate, not a review-only generator. From da58168d7184ead53b65d2ec8fdbf04e2fca536c Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 06:30:30 -0400 Subject: [PATCH 6/6] docs(ui): scope the last two flat override-count claims (RIG-2154) Fourth review pass found the conditional rewrite reached four of six base-dependent sites and left two stating the override set as a flat fact. - The Threshold opening said "the 2 smallest shots need a per-shot widening" unqualified while qualifying only the second half of the same sentence, which by contrast implied the count is base-independent. It is not: the count is 3 at base 0.0005, 2 at 0.001 and 1 at 0.002. Now scoped to the recommended base with the three counts stated. - T1 described "the two per-shot overrides" as a fixed pair. Reworded to the overrides the ruled base ratio yields, noting two at the recommended 0.001. Both are completions of the previous commit's own edit rather than new claims; no arithmetic changed and OQ-1 remains unruled. Co-authored-by: Matt Wilkinson --- .../ui/compass-visual-regression-gate/design.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/designs/ui/compass-visual-regression-gate/design.md b/docs/designs/ui/compass-visual-regression-gate/design.md index ee1a5617..8f080dd3 100644 --- a/docs/designs/ui/compass-visual-regression-gate/design.md +++ b/docs/designs/ui/compass-visual-regression-gate/design.md @@ -124,12 +124,12 @@ spans ~4 orders of magnitude: a full-page shot (~1280×720+, ≥900 K px) at 0.001 allows ~900 differing pixels, while `state-dot.png` (9×10 = 90 px) gets a budget of 0.09 px — effectively byte-exact, the *least* slack on the shot most exposed to a single anti-aliasing pixel shift after a Chromium bump. One -ratio cannot serve both ends, so the 2 *smallest* shots (`state-dot`, -`bridge-card`) need a per-shot widening. At the recommended base the two +ratio cannot serve both ends, so at the recommended base the 2 *smallest* +shots (`state-dot`, `bridge-card`) need a per-shot widening, while the two larger close-ups (`bridge-colheads`, `right-sidebar`) do not: their area-scaled budgets @0.001 are already 35 px and 260 px, comfortably above -the intended slack. Whether `bridge-colheads` still qualifies is -base-dependent — see the per-shot list below. +the intended slack. Which shots qualify is base-dependent — the count is 3 +at 0.0005, 2 at 0.001 and 1 at 0.002; see the per-shot list below. **The widening knob is a per-shot `maxDiffPixelRatio`, not `maxDiffPixels`.** At the 1.62.1 pin the two pixel-count knobs resolve with `Math.min`, not max: @@ -321,8 +321,9 @@ as OQ-5 with this recommendation since the issue asks. Extend `apps/ui/playwright.config.ts` with: `snapshotPathTemplate: "{testDir}/__screens__/{arg}{ext}"` and `expect: { toHaveScreenshot: { maxDiffPixelRatio: } }` — the config-level -default (base ratio per OQ-1, recommendation 0.001); the two per-shot -`maxDiffPixelRatio` overrides are set at their call sites in T2, not here. +default (base ratio per OQ-1, recommendation 0.001); the per-shot +`maxDiffPixelRatio` overrides that base ratio yields (two at the recommended +0.001) are set at their call sites in T2, not here. `threshold` is left unset (default 0.2) as a recorded decision. No project or webServer changes — the determinism knobs at :57-72 and the fixture-mode webServer at :79-95 are already the