From 9c723aaae9c37c8a7d87b31567c413d92897bd4a Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Fri, 12 Jun 2026 20:20:33 -0500 Subject: [PATCH] chore(docs): sweep stale visual-baseline references + queue gate-hardening convoy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #58 (83a358b, 2026-06-02) committed the first Linux visual baseline, resolving the seed-visual-baselines-on-linux convoy. But the cleanup sweep across docs that referenced the convoy as "queued / not yet done" never landed. Three files still describe the world as if PR #58 hadn't happened, which confuses any agent reading the codebase to understand the visual-diff pipeline: 1. tests/visual/homepage.spec.ts module docblock — described "FIRST RUN (no committed baseline yet)" and "SEEDING THE BASELINE (post-merge follow-up)" as the active state. 2. playwright.config.js snapshotPathTemplate comment — said "Per Decision 4, we don't commit baselines this convoy" and pointed at the queued seed convoy as future work. 3. AGENTS.md § Testing § Visual baselines + § CI behavior § Screenshot diff — claimed "none committed yet" and that the first visual-diff run "will fail at the test step because no baseline exists yet." Sweeping all three to describe the current reality. The Mac-vs-Linux platform footgun (snapshotPathTemplate has no {platform} token) is still live, so that warning stays — just rephrased from "don't have baselines yet" to "don't regenerate them on a Mac." Also surfaces a separate finding the seed work left behind: visual-diff.yml's screenshot capture step still carries `continue-on-error: true`, making the diff advisory rather than gating. Flipping it requires re-seeding the baseline against post-glass-redesign main first (the PR #58 baseline predates unify-glass-panel-surfaces + cleanup-card-item-list-and-share-modal-palette + migrate-button-input-mobilenav-to-glass-primitive). New convoy seed at .convoys/harden-visual-diff-gate.md captures the two-step shape (re-seed baseline, then flip the gate) plus the recommended workflow_dispatch approach for repeatable re-seeding on CT 111. No code behavior changes. Documentation + .md convoy seed only. Co-authored-by: Cursor --- .convoys/harden-visual-diff-gate.md | 112 ++++++++++++++++++++++++++++ AGENTS.md | 26 ++++--- playwright.config.js | 20 ++--- tests/visual/homepage.spec.ts | 40 ++++++---- 4 files changed, 164 insertions(+), 34 deletions(-) create mode 100644 .convoys/harden-visual-diff-gate.md diff --git a/.convoys/harden-visual-diff-gate.md b/.convoys/harden-visual-diff-gate.md new file mode 100644 index 0000000..dc48a6b --- /dev/null +++ b/.convoys/harden-visual-diff-gate.md @@ -0,0 +1,112 @@ +--- +slug: harden-visual-diff-gate +status: queued +opened: 2026-06-12 +owner: rstillw +prerequisites: + - Stable `main` post-`unify-glass-panel-surfaces` / + `cleanup-card-item-list-and-share-modal-palette` / + `migrate-button-input-mobilenav-to-glass-primitive` (all merged) + - CT 111 self-hosted runner online (resolved by PR #132, 2026-06-07) +related: + - PR #58 (`83a358b`) — initial Linux baseline seeded + - PR #18 (`7b6f751`) — `adopt-playwright-smoke` Decision 4: `continue-on-error` end state + - `tests/visual/homepage.spec.ts` module docblock + - `.github/workflows/visual-diff.yml` line ~126 +--- + +# harden-visual-diff-gate + +## Problem + +`.github/workflows/visual-diff.yml` still carries `continue-on-error: true` +on the "Capture screenshots (PR)" step. This was the documented end state +of `adopt-playwright-smoke` (Decision 4) because no Linux baseline existed +yet. PR #58 seeded the baseline on 2026-06-02, but the gate was never +flipped — visual drift continues to ship as artifacts + PR comments only, +not as a blocking check. + +Additionally, the committed baseline (`tests/visual/__screenshots__/home.png`) +was generated against `main` at `83a358b`, which is several convoys behind +the current homepage rendering (glass redesign briefs, corner-light tone +adjustments, card-class retirement, etc.). Flipping `continue-on-error` +today would fail every PR touching `pages/**` / `components/**` / `styles/**` +against this stale reference. + +## Two-step shape + +This is one convoy with a strict ordering constraint: + +### Step 1 — Re-seed the baseline against current `main` + +Generate a fresh `home.png` from the current homepage rendering on Linux +(byte-equivalent to CT 111's runner output). Two options: + +- **Option A: Ad-hoc on CT 111.** SSH the homelab, exec into a runner + container, run `BASE_URL=https://deckhearth.com VERCEL_AUTOMATION_BYPASS_SECRET= npm run test:visual:update`, + scp the new PNG back, commit it. Most direct. +- **Option B (recommended): New `seed-visual-baselines.yml` workflow.** + `workflow_dispatch`-only, runs on `[self-hosted, axiom]`, takes + `BASE_URL` as an input, runs `--update-snapshots`, uploads the + regenerated `tests/visual/__screenshots__/**` as an artifact for the + operator to download + commit. Repeatable; no SSH dance. + +Either option ships a PR with the new baseline PNG. The PR should also +clear the "Known staleness" callout in `AGENTS.md` § Testing § Visual +baselines. + +### Step 2 — Flip the gate + +After Step 1's PR merges and a follow-up PR confirms `Screenshot diff` +passes cleanly against the new baseline: + +- Remove `continue-on-error: true` from + `.github/workflows/visual-diff.yml` (line ~126). +- Update `AGENTS.md` § CI behavior § Screenshot diff to drop the + "advisory, not gating" language. +- Update the module docblock in `tests/visual/homepage.spec.ts` to + drop the "still carries `continue-on-error: true`" callout. +- Optional: bump `expect.toHaveScreenshot.maxDiffPixelRatio` in + `playwright.config.js` if some pixel jitter is unavoidable (e.g. + font subpixel rendering across runner image bumps). Default is 0 + (any pixel diff fails); 0.01 (1%) is a common safe floor for + Chromium-vs-Chromium runs. + +## Decisions to ratify + +- **D1.** Option B (new `seed-visual-baselines.yml` workflow) vs Option A + (ad-hoc). Recommend B for repeatability — the homepage will continue + to evolve and re-seeding will be a recurring operation, not a + one-shot. +- **D2.** `maxDiffPixelRatio` value. Defer to operator preference; 0 is + the strictest and what we currently use implicitly via Playwright + defaults. 0.001-0.01 is a reasonable cushion. +- **D3.** Do we also harden `preview-smoke.yml`? It already runs as a + blocking gate (no `continue-on-error`); no change needed. This convoy + is scoped to `visual-diff.yml` only. + +## Risks + +| # | Risk | Mitigation | +|---|---|---| +| 1 | Re-seeded baseline drifts on next runner-image update | Watchtower's weekly update cycle (CT 111) could shift Chromium subpixel rendering. Mitigate via D2's `maxDiffPixelRatio` cushion | +| 2 | Step 1's baseline PR fails its own `Screenshot diff` | Expected — the new baseline IS the visual change. Use `pipeline: skip visual` directive in the PR body OR open the baseline PR with the `skip-metrics` label equivalent for visual-diff (currently none — would need a new bypass mechanism). Cleaner: land the baseline PR via the `tests/visual/**` path filter, which DOES trigger visual-diff but the new screenshot vs new baseline should match by construction | +| 3 | Step 2 lands but a third party reverts `continue-on-error` later | Add a 9th check to `forbidden-patterns` in `ci.yml`: `grep -n 'continue-on-error' .github/workflows/visual-diff.yml` should return zero | + +## Non-goals + +- Adding more visual baselines (login page, dashboard, etc.) — out of + scope. The single homepage baseline is the smoke test of the + visual-diff pipeline; deeper coverage is per-feature work. +- Switching to a hosted visual-regression service (Percy, Chromatic, + Argos) — handles the platform problem cleanly but adds a paid + dependency. Local + axiom is free and works. + +## Acceptance + +- `visual-diff.yml` gates merge (failed diff = red required check). +- Baseline regenerable via a reviewable Git-native workflow (no SSH + required, no Mac-overwrite-Linux footgun). +- `AGENTS.md` + `tests/visual/homepage.spec.ts` docblock no longer + describe the gate as advisory. +- Optional 9th forbidden-patterns check locks the gate in place. diff --git a/AGENTS.md b/AGENTS.md index cc14087..fa04cb4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,21 +150,27 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560 Local-run convention: boot `next dev` in one terminal, then in another run `BASE_URL=http://localhost:3000 npm run test:smoke` (or against a deployed preview, `BASE_URL=https://.vercel.app VERCEL_AUTOMATION_BYPASS_SECRET= npm run test:smoke`). No `next dev` auto-boot in the test scripts (Decision 6 of `adopt-playwright-smoke`). - **Browsers must be installed once locally:** `npx playwright install --with-deps chromium`. CI re-runs this on every workflow run (it's cached when possible). -- **Visual baselines:** none committed yet. `tests/visual/__screenshots__/` is intentionally absent and intentionally NOT in `.gitignore` (baselines, when they exist, must be committed). First-run baseline generation MUST happen in a Linux environment so the PNG matches what CI produces. Recommended path is the Playwright Docker image: +- **Visual baselines:** committed under `tests/visual/__screenshots__/`. The initial Linux baseline (`home.png`) was seeded by PR #58 (`83a358b`, 2026-06-02). Baselines are committed to git — they are not gitignored — so a `Screenshot diff` failure is reviewable from PR comments + artifacts without bouncing through a regeneration step. **Re-seeding** (when the homepage changes intentionally) MUST happen in a Linux environment so the PNG matches what CI produces. Recommended paths: - ``` - docker run --rm -v "$PWD":/work -w /work \ - mcr.microsoft.com/playwright:v1.60.0-noble \ - sh -c "npm ci && BASE_URL= \ - VERCEL_AUTOMATION_BYPASS_SECRET= \ - npm run test:visual:update" - ``` + 1. **Playwright Docker image** (works from any host): - Mac-generated baselines will NOT match Linux CI — `playwright.config.js`'s custom `snapshotPathTemplate` has no `{platform}` token, so a Mac update silently overwrites the canonical Linux baseline. Tracked as the queued `seed-visual-baselines-on-linux` convoy (see `.convoys/ship-readiness.md` § Queued convoys). + ``` + docker run --rm -v "$PWD":/work -w /work \ + mcr.microsoft.com/playwright:v1.60.0-noble \ + sh -c "npm ci && BASE_URL= \ + VERCEL_AUTOMATION_BYPASS_SECRET= \ + npm run test:visual:update" + ``` + + 2. **CT 111 directly** (preferred when iterating — same toolchain as the diff workflow, byte-equivalent output). Dispatched via the (queued) `seed-visual-baselines` workflow once it lands; until then, `pct exec 111 -- docker exec gha-runner-1 sh -c "...`" works ad-hoc. + + Mac-generated baselines will NOT match Linux CI — `playwright.config.js`'s custom `snapshotPathTemplate` has no `{platform}` token, so a Mac update silently overwrites the canonical Linux baseline. **Never run `npm run test:visual:update` on a Mac** unless you immediately throw the result away. + + **Known staleness as of 2026-06-12:** the committed `home.png` was seeded against `main` at `83a358b` (pre-`unify-glass-panel-surfaces`, pre-`cleanup-card-item-list-and-share-modal-palette`, pre-`migrate-button-input-mobilenav-to-glass-primitive`). Until it is re-seeded, `Screenshot diff` is advisory only — see the next bullet for why `continue-on-error: true` is still in place on `visual-diff.yml`. - **CI behavior:** - **Vitest:** the `test:` job in `.github/workflows/ci.yml` runs `npm run test:run` on every PR and push to `main` and is **blocking** (no `|| true`, no `continue-on-error`). A red test job blocks merge. - **Playwright smoke:** runs on every PR via `preview-smoke.yml`. Gate skip via `pipeline: skip smoke` in the PR body (handled in the `gate:` job's Decide step via env-var routing — see § 7's shell-injection note). Pre-migration runtime: 59s end-to-end on `ubuntu-latest` (PR #18 post-merge run). Post-migration on the axiom pool: cold-cache first run ~6 min (Chromium download); warm cache thereafter ~1–2 min. - - **Screenshot diff:** runs only on PRs touching `pages/**` / `components/**` / `styles/**` / `tailwind.config.js` / `postcss.config.js` via `visual-diff.yml`. **First `Screenshot diff` run after `adopt-playwright-smoke` will fail at the test step** because no baseline exists yet; `continue-on-error: true` swallows the failure and the comment-on-PR step posts "Visual Diff — view run" with empty artifacts. That is the documented Decision-4 end state of `adopt-playwright-smoke`, not a regression — it stays that way until `seed-visual-baselines-on-linux` lands. + - **Screenshot diff:** runs only on PRs touching `pages/**` / `components/**` / `styles/**` / `tailwind.config.js` / `postcss.config.js` (and `tests/visual/**` for baseline updates) via `visual-diff.yml`. The committed Linux baseline at `tests/visual/__screenshots__/home.png` (PR #58, `83a358b`) means the test now exercises a real comparison rather than fast-failing on a missing file. The screenshot-capture step still carries `continue-on-error: true`, so the diff is **advisory, not gating**: artifacts + a "Visual Diff — view run" PR comment surface drift, but a red run does not block merge. Flipping that to a hard gate is the queued `harden-visual-diff-gate` follow-up — it requires re-seeding the baseline against post-`unify-glass-panel-surfaces` `main` first, otherwise every UI-touching PR fails the new gate against a stale reference image. - **CI minute optimizations (slash-ci-minutes convoy, 2026-06-04):** - **Doc-only PRs skip ALL of ci.yml + preview-smoke.yml.** Both workflows carry `paths-ignore` for `.convoys/**`, `**/*.md`, `docs/**`, `AGENTS.md`, `.cursor/**`, and `README.md`. A pure-docs PR triggers zero GitHub Actions jobs (Vercel still builds — it's not on the Actions billing). `visual-diff.yml` was already cost-conscious via a positive `paths:` allowlist and is unchanged. - **The 6 grep-only forbidden-* jobs collapsed into one.** They previously ran as 6 independent jobs (each with its own `actions/checkout`); the consolidated `forbidden-patterns` job runs all 6 checks as labeled `::group::` sections in a single bash step, with a FAIL flag at the bottom so every violation across all 6 checks still surfaces in one run (same diagnostic behavior, ~5/6 of the per-PR checkout overhead removed). The 6 original job names (`forbidden-endpoints`, `forbidden-cors-headers`, `forbidden-client-side-llm-keys`, `forbidden-modal-shell-without-primitive`, `forbidden-deprecated-color-aliases`, `forbidden-stale-strings`) no longer appear in the checks list — references in this file (e.g. CI job `forbidden-stale-strings` blocks ...) are now informational, not check-name lookups. `pr-health-rollup.yml` was unaffected because it only looks up `Lint` and `Schema map up to date` by name. diff --git a/playwright.config.js b/playwright.config.js index 10759d4..b7acb4b 100644 --- a/playwright.config.js +++ b/playwright.config.js @@ -76,18 +76,20 @@ export default defineConfig({ reporter: IS_CI ? [['list'], ['html', { open: 'never' }]] : 'list', // Visual baselines live at `tests/visual/__screenshots__/{ext}`. - // Workflow `visual-diff.yml` line 94 uploads this exact path as the - // artifact — keep them aligned. {arg} is the snapshot name from + // Workflow `visual-diff.yml` uploads this exact path as the artifact — + // keep them aligned. {arg} is the snapshot name from // `toHaveScreenshot('home.png')` without the extension; {ext} is // the extension with the leading dot. // - // Cross-platform note (Risk R3): this template drops Playwright's - // default `--` suffix. That means a Mac dev - // running `npm run test:visual:update` overwrites the Linux-CI - // baseline. Per Decision 4, we don't commit baselines this convoy; - // operator seeds via `mcr.microsoft.com/playwright:v1.60.0-noble` - // Docker on Linux. Multi-platform support is the - // `seed-visual-baselines-on-linux` follow-up convoy's job. + // Cross-platform CAUTION (still live): this template drops Playwright's + // default `--` suffix. A Mac dev running + // `npm run test:visual:update` silently overwrites the Linux-CI + // baseline with a Mac-rendered PNG that the CT 111 runner will reject + // on the next diff. The seeded Linux baseline (PR #58 `83a358b`, + // 2026-06-02) is the canonical artifact. Re-seed via the Linux Docker + // recipe in `tests/visual/homepage.spec.ts`'s module docblock OR + // dispatch the (queued) `seed-visual-baselines` workflow on CT 111. + // Never via `npm run test:visual:update` on a Mac. snapshotPathTemplate: 'tests/visual/__screenshots__/{arg}{ext}', use: { diff --git a/tests/visual/homepage.spec.ts b/tests/visual/homepage.spec.ts index 9cab621..8d6d220 100644 --- a/tests/visual/homepage.spec.ts +++ b/tests/visual/homepage.spec.ts @@ -3,20 +3,27 @@ import { test, expect } from '@playwright/test'; /** * Visual baseline for the public homepage. * - * FIRST RUN (no committed baseline yet): - * The Screenshot diff workflow runs `playwright test --project=visual - * --update-snapshots=none` (per .github/workflows/visual-diff.yml). With - * no baseline file at `tests/visual/__screenshots__/home.png` AND the - * `none` flag, this test FAILS — and that's the documented end state of - * the `adopt-playwright-smoke` convoy (Decision 4 in - * `.convoys/adopt-playwright-smoke.md`). The workflow's - * `continue-on-error: true` swallows the failure and the comment-on-PR - * step posts "Visual Diff — view run" with empty artifacts. + * The Screenshot diff workflow runs `playwright test --project=visual + * --update-snapshots=none` (per .github/workflows/visual-diff.yml). The + * `none` flag means "use the existing baseline; do not write a new one" + * — drift between the rendered page and the committed PNG fails the + * test. The committed Linux baseline lives at + * `tests/visual/__screenshots__/home.png` (seeded by PR #58 `83a358b`, + * 2026-06-02). * - * SEEDING THE BASELINE (post-merge follow-up): - * Run `npm run test:visual:update` in a Linux environment so the - * generated PNG matches what CI will produce. The cleanest path is the - * Playwright Docker image: + * NOTE: as of this writing, `visual-diff.yml`'s screenshot capture step + * still carries `continue-on-error: true` — failures surface as + * artifacts + PR comments but do not gate merge. Flipping that to a + * hard gate is the `harden-visual-diff-gate` follow-up; it depends on + * the baseline being re-seeded against the post-glass-redesign main + * (the seeded PR-#58 PNG predates the `unify-glass-panel-surfaces` / + * `cleanup-card-item-list-and-share-modal-palette` / + * `migrate-button-input-mobilenav-to-glass-primitive` work). + * + * RE-SEEDING THE BASELINE (when the homepage changes intentionally): + * Run on Linux — Mac-generated PNGs do not match CT 111 Linux output + * because `playwright.config.js`'s `snapshotPathTemplate` has no + * `{platform}` token. Recommended path is the Playwright Docker image: * * docker run --rm -v "$PWD":/work -w /work \ * mcr.microsoft.com/playwright:v1.60.0-noble \ @@ -24,8 +31,11 @@ import { test, expect } from '@playwright/test'; * VERCEL_AUTOMATION_BYPASS_SECRET= \ * npm run test:visual:update" * - * Then commit `tests/visual/__screenshots__/home.png`. This is tracked - * as the `seed-visual-baselines-on-linux` follow-up convoy. + * …or dispatch the (queued) `seed-visual-baselines` workflow on + * CT 111 directly, which uses the same Linux toolchain as the + * diff workflow so byte-equivalence is guaranteed. + * + * Commit the regenerated `tests/visual/__screenshots__/home.png`. */ const BASE = process.env.BASE_URL ?? 'http://localhost:3000'; -- 2.45.2