Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -662,17 +662,19 @@ jobs:
# - Pixel comparison needs a soak before it can block a merge; this repo has
# already paid for a sub-pixel rounding flake (the min-h-11 -> min-h-12 change).
# Promote to a required check by adding it to `pr-required` once the baselines have
# held across a few runs, and drop `continue-on-error` at the same time.
# held across a few runs, and drop the pixel-step soft-fail / drift classifier then.
visual-baseline:
name: Visual baselines (advisory)
needs: changes
# Deliberately NOT on pull_request or merge_group. Owner decision, 2026-08-09:
# a pixel gate that reports on every UI pull request taxes exactly the work it
# exists to support. During a redesign the honest result is red on every push,
# and the baselines cannot be refreshed until a run has produced the new
# images, so that red is unavoidable rather than actionable. It never blocked
# a merge (`continue-on-error`, and absent from `pr-required`), but a red check
# nobody can act on mid-iteration is friction whether or not it gates.
# images, so that red is unavoidable rather than actionable. Pixel drift is
# soft-failed at the comparison step (and classified) rather than with a
# job-level continue-on-error that would also hide setup failures; the job
# remains absent from `pr-required`. A red check nobody can act on
# mid-iteration is friction whether or not it gates.
# `merge_group` is excluded for the same reason: queue runs are still
# pre-merge UI churn, not a post-land refresh point.
#
Expand All @@ -687,7 +689,8 @@ jobs:
if: >
needs.changes.outputs.ui_changed == 'true' &&
contains(fromJSON('["push","schedule","workflow_dispatch"]'), github.event_name)
continue-on-error: true
# Deliberately NOT continue-on-error at the job: setup / artifact / non-drift
# failures must stay red. Only the pixel-comparison step soft-fails below.
runs-on: ubuntu-24.04
timeout-minutes: 45
steps:
Expand All @@ -700,8 +703,29 @@ jobs:
uses: ./.github/actions/setup-ui-e2e

- name: Chromium visual baselines
id: visual-comparison
# Pixel drift is evidence, not an execution failure. Keeping this at the
# step (rather than only the job) prevents GitHub from publishing a red
# check for the expected stale-golden case demonstrated by PR #1743.
continue-on-error: true
run: npm run test:e2e:visual
Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

- name: Report visual differences
if: steps.visual-comparison.outcome == 'failure'
run: |
if ! node scripts/classify-visual-baseline-outcome.mjs; then
echo "::error title=Visual baseline non-drift failure::Visual comparison failed for a reason other than pixel drift (missing baseline, setup/runtime, or assertion). See the visual-baseline-${{ github.run_id }} artifact."
exit 1
fi
echo "::warning title=Visual baseline drift::Chromium pixels differ from the committed Linux baseline. Review the visual-baseline-${{ github.run_id }} artifact; this advisory comparison does not block merging."
{
echo "### Visual baseline drift detected"
echo
echo "The Chromium comparison found changed pixels. This is advisory: required interaction, accessibility, and style-contract checks remain separate."
echo
echo "Download \`visual-baseline-${{ github.run_id }}\` and review the expected, actual, and diff images before refreshing the Linux baseline."
} >> "$GITHUB_STEP_SUMMARY"

# The actual/diff/expected PNGs live here. On a first run these are the images
# to commit as the baseline; on a later run they are the evidence of what moved.
- name: Upload visual diffs
Expand Down
1 change: 1 addition & 0 deletions docs/branch-review-ledger.md
Original file line number Diff line number Diff line change
Expand Up @@ -855,4 +855,5 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie
| 2026-08-09 | PR #1782 / cursor/fix-document-open-scroll-e5bf | 5709f2cc7a954197e02107c96d7896d8d13445c3 | document-viewer open-at-top | ship: remove chunk mount scrollIntoView so document opens stay at overview top | document-viewer-shell.dom 7 pass; document-section-summary.dom 8 pass; verify:pr-local dry-run |
| 2026-08-09 | cursor/fix-document-open-scroll-e5bf (PR #1782) | 98029875db7d640d3e699829249bb33892296bff | PR #1782 unblock | before: static-pr+coverage failed on stale adoption-manifest (document-viewer-shell testFiles drift), merge-tree clean 0 behind, auto-merge armed, 1 advisory CodeRabbit waitFor thread; after: regenerated adoption-manifest, hardened scroll negative assertion, pre-commit+handoff adoption sync to prevent recurrence; CodeRabbit dispositioned as fixed by sync assert | check:design-system-adoption PASS; vitest design-system-adoption+document-viewer-shell+docs-inventory 63/63 PASS; format; no provider-backed checks |
| 2026-08-09 | cursor/fix-document-open-scroll-e5bf (PR #1782) | 86698228533ebe10452c10c1bd7a3e1610d891ae | PR #1782 unblock | merged origin/main (behind-but-clean); fixed static-pr TS2322 on document-viewer-shell chunk fixture; fixed Production UI DSM compare remove stall via location.assign + DOM proof; prior adoption-manifest drift already fixed | tsc clean for changed files; vitest document-viewer-shell+dsm-compare-remove+design-system-adoption 59/59 PASS; check:design-system-adoption PASS; format; no provider-backed checks |
| 2026-08-10 | codex/visual-baseline-advisory-pr | 6bc57714c36bc6d027561bb8f5f8b00bb92524b2 | PR #1791 babysit unblock | fixed Production UI formulation Clear→Draft flake settle; classified visual drift vs non-drift failures | test:ci-workflows 263; classify-visual-baseline-outcome+ci-cache-safety 40 |
| 2026-08-10 | PR #1797 / claude/codex-m4a-retire-dead-type-8wq9ta | 6bf3c7b2a0600021290e165302fd07d721af6592 | retire the dead --text-2xl-compact type step (ledger #297): globals.css @theme, twMerge config, two test lists, the design-system-contract exemption, TOKENS.md/GATES.md | Executed the recorded next action on outstanding-issues #297. The step had zero class-utility and zero var(--text-*) consumers, so the deletion renders identically; UNUSED_TYPE_STEP_EXEMPTIONS is now empty and the declared-but-unconsumed gate holds the line with no carve-out. One test fixture using the token as a synthetic var() consumer was repointed at --text-2xl-minus. GATES.md corrected to eight non-standard steps; the 705-consumer total is unchanged because this step contributed 0. No clinical, RAG-ranking or operational risk paths touched (classifyPullRequestFiles: all false). | check:design-system-contract PASS (705 production files); check:type-scale --strict PASS; lint exit 0; typecheck exit 0; npm run build after rm -rf .next exit 0 (Compiled successfully in 63s); check:outstanding-issues PASS; verify:pr-local completed through typecheck then failed at test on a PRE-EXISTING root-permission failure in tests/pr-handoff-stop.test.ts that reproduces on clean d812c76 (5993 passed, 1 failed); build and check:rag:fixtures run/assessed separately. No UI gate: no rendered output can change. No provider-backed check run. |
2 changes: 1 addition & 1 deletion docs/scripts-index.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Scripts index

Curated map of `scripts/` (224 files) and the `package.json` script surface (235 entries),
Curated map of `scripts/` (225 files) and the `package.json` script surface (235 entries),
grouped by purpose. This is orientation, not an exhaustive per-file listing — the authoritative
command list is `package.json`, and `npm run docs:check-scripts` verifies every `npm run <x>`
referenced in docs resolves to a real script. `npm run docs:update` refreshes the exact counts above.
Expand Down
19 changes: 16 additions & 3 deletions docs/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,13 @@ job's artifact, not from a developer machine** — font hinting and antialiasing
laptop-generated baseline makes every CI run red. A platform with no baseline fails loudly rather
than passing silently. The CI `visual-baseline` job is deliberately **off `pull_request` and
`merge_group`** (owner decision on PR #1755 / `#118`): it still runs on pushes to main/release, the
weekly schedule, and `workflow_dispatch`, and stays `continue-on-error` / outside `pr-required`. Do
not re-add pre-merge triggers or promote it without an explicit owner ask.
weekly schedule, and `workflow_dispatch`, and stays outside `pr-required`. Do not re-add pre-merge
triggers or promote it without an explicit owner ask. Only the pixel-comparison step uses
`continue-on-error`, and only after `scripts/classify-visual-baseline-outcome.mjs` confirms the
failure is a `toHaveScreenshot` pixel mismatch: drift creates a workflow warning, job summary, and
downloadable expected/actual/diff artifact instead of a failed check. Missing baselines, setup,
runtime/assertion, and artifact-publication failures remain visible as job failures because those
runs produced no trustworthy comparison evidence.

## Performance budget

Expand Down Expand Up @@ -266,7 +271,15 @@ UI scope runs a fail-fast `@critical` Chromium job on pull requests, then requir

PR body synchronization is skipped unless the checked-out head actually contains `PR_POLICY_BODY.md`. The eval-canary liveness API probe runs once with the daily Ops Digest cadence rather than on every PR. These remove repeated provider-side work without weakening a required result.

Two further jobs are advisory (`continue-on-error`, deliberately outside `pr-required`): `visual-baseline` on UI scope and `lighthouse-budget` on the narrower perf scope (see "When the budget runs" above — `worker/**` and container surfaces, dependency manifests and the lockfile, Playwright/test surfaces, most of `src/app/api/**` other than initial-load handlers, and `src/app/mockups/**` are excluded; `src/proxy.ts` stays in). Both upload their evidence on every run, pass or fail, because the artifact is the whole point on a first run — the baselines to adopt and the reports to grade. Promote either to required by adding it to `pr-required` and removing `continue-on-error` in the same edit; for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`.
Two further jobs are advisory (deliberately outside `pr-required`): `visual-baseline` on UI scope
(soft-fail only the classified pixel-drift step) and `lighthouse-budget` on the narrower perf scope
(`continue-on-error` — see "When the budget runs" above; `worker/**` and container surfaces,
dependency manifests and the lockfile, Playwright/test surfaces, most of `src/app/api/**` other than
initial-load handlers, and `src/app/mockups/**` are excluded; `src/proxy.ts` stays in). Both upload
their evidence on every run, pass or fail, because the artifact is the whole point on a first run —
the baselines to adopt and the reports to grade. Promote either to required by adding it to
`pr-required` and removing the soft-fail (`continue-on-error` / drift classifier) in the same edit;
for `lighthouse-budget` that edit must also restore `merge_group` to its `if:`.

## Contribution checklist (UI changes)

Expand Down
154 changes: 154 additions & 0 deletions scripts/classify-visual-baseline-outcome.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
#!/usr/bin/env node
/**
* Decide whether a failed `test:e2e:visual` run was pixel drift (advisory) or an
* infrastructure / non-comparison failure that must stay red.
*
* Exit 0 → advisory drift only (or no failures found in an existing report).
* Exit 1 → missing report, missing baselines, runtime/assertion failures, or mixed.
*/
import { existsSync, readFileSync, readdirSync } from "node:fs";
import { join } from "node:path";
import { pathToFileURL } from "node:url";

const DEFAULT_JUNIT = "test-results/playwright-junit.xml";
const DEFAULT_RESULTS = "test-results/playwright-results.json";

const decode = (value) =>
value
.replaceAll("&quot;", '"')
.replaceAll("&apos;", "'")
.replaceAll("&lt;", "<")
.replaceAll("&gt;", ">")
.replaceAll("&amp;", "&");

const attribute = (attributes, name) => {
const match = attributes.match(new RegExp(`\\b${name}="([^"]*)"`));
return match ? decode(match[1]) : "";
};

/** Failure/error bodies from a Playwright JUnit report. */
export function failureBodiesFromJunit(xml) {
return [...xml.matchAll(/<testcase\b([^>]*)>([\s\S]*?)<\/testcase>/g)].flatMap((match) => {
const title = attribute(match[1], "name");
const classname = attribute(match[1], "classname");
return [...match[2].matchAll(/<(?:failure|error)\b([^>]*)>([\s\S]*?)<\/(?:failure|error)>/g)].map((failure) => ({
title,
classname,
message: attribute(failure[1], "message"),
body: decode(failure[2]).trim(),
}));
});
}

function collectErrorMessages(node, out = []) {
if (!node || typeof node !== "object") return out;
if (Array.isArray(node)) {
for (const item of node) collectErrorMessages(item, out);
return out;
}
if (node.error?.message) out.push(String(node.error.message));
if (Array.isArray(node.errors)) {
for (const error of node.errors) {
if (error?.message) out.push(String(error.message));
}
}
for (const value of Object.values(node)) collectErrorMessages(value, out);
return out;
}

export function failureMessagesFromResults(resultsJson) {
return collectErrorMessages(resultsJson);
}

/** Pixel-drift only: toHaveScreenshot mismatch with an existing baseline. */
export function isPixelDriftFailure(text) {
const haystack = String(text ?? "");
if (!haystack) return false;
if (/snapshot doesn't exist/i.test(haystack)) return false;
if (/AWAITING_BASELINE/i.test(haystack)) return false;
return /toHaveScreenshot/i.test(haystack) || /Screenshot comparison failed/i.test(haystack);
}

function listDiffPngs(root = "test-results") {
if (!existsSync(root)) return [];
const found = [];
const walk = (dir) => {
for (const entry of readdirSync(dir, { withFileTypes: true })) {
const full = join(dir, entry.name);
if (entry.isDirectory()) walk(full);
else if (entry.isFile() && /-diff\.png$/i.test(entry.name)) found.push(full);
}
};
walk(root);
return found;
}

export function classifyVisualBaselineOutcome({
junitPath = DEFAULT_JUNIT,
resultsPath = DEFAULT_RESULTS,
testResultsDir = "test-results",
} = {}) {
const hasJunit = existsSync(junitPath);
const hasResults = existsSync(resultsPath);
if (!hasJunit && !hasResults) {
return {
kind: "infrastructure",
reason: "No Playwright JUnit or JSON report after visual comparison failure.",
};
}

const messages = [];
if (hasJunit) {
for (const failure of failureBodiesFromJunit(readFileSync(junitPath, "utf8"))) {
messages.push([failure.message, failure.body, failure.title].filter(Boolean).join("\n"));
}
}
if (hasResults) {
messages.push(...failureMessagesFromResults(JSON.parse(readFileSync(resultsPath, "utf8"))));
}

const unique = [...new Set(messages.map((message) => message.trim()).filter(Boolean))];
if (unique.length === 0) {
const diffs = listDiffPngs(testResultsDir);
if (diffs.length > 0) {
return {
kind: "pixel-drift",
reason: `Found ${diffs.length} screenshot diff artifact(s) without parsed failure text.`,
};
}
return {
kind: "infrastructure",
reason: "Visual comparison failed but the report contains no failed testcases.",
};
}

const nonDrift = unique.filter((message) => !isPixelDriftFailure(message));
if (nonDrift.length > 0) {
return {
kind: "non-drift",
reason: `Non-comparison failure(s) present (${nonDrift.length}/${unique.length}).`,
samples: nonDrift.slice(0, 3),
};
}

return {
kind: "pixel-drift",
reason: `All ${unique.length} failure(s) are toHaveScreenshot pixel mismatches.`,
};
}

function main() {
const outcome = classifyVisualBaselineOutcome();
if (outcome.kind === "pixel-drift") {
console.log(`Visual baseline outcome: pixel-drift — ${outcome.reason}`);
process.exitCode = 0;
return;
}
console.error(`Visual baseline outcome: ${outcome.kind} — ${outcome.reason}`);
for (const sample of outcome.samples ?? []) {
console.error(`- ${sample.split("\n")[0].slice(0, 200)}`);
}
process.exitCode = 1;
}

if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) main();
8 changes: 6 additions & 2 deletions tests/__screenshots__/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,5 +54,9 @@ without `--update-snapshots`: a baseline that fails its own immediate re-run is
capturing a race, not a design. `maxDiffPixelRatio: 0.002` with `threshold: 0.2`
absorbs antialiasing, not layout that has not settled.

Comparison is advisory until the jobs are flipped off `continue-on-error`. Until
then a real regression shows as a red _artifact_, not a red _check_.
Comparison is advisory. Pixel drift produces a workflow warning and summary while
the job uploads expected, actual, and diff images for review; it does not produce a
failed GitHub check. Missing baselines and other non-comparison failures stay red.
The job runs post-land, weekly, or manually rather than on pull requests and merge
queues. Setup or artifact-upload failures can still fail the job because they leave
no trustworthy evidence to review.
18 changes: 17 additions & 1 deletion tests/ci-cache-safety.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -347,11 +347,27 @@ describe("Visual baseline routing", () => {
// Owner decision (PR #1755 / #118): pre-merge UI churn is the wrong place for
// an unavoidably-red pixel gate. merge_group is still pre-merge.
expect(visualBaselineJob).toContain('["push","schedule","workflow_dispatch"]');
expect(visualBaselineJob).toContain("continue-on-error: true");
const prRequiredNeeds = /\n pr-required:\n[\s\S]*?needs:\s*\n?\s*\[([\s\S]*?)\]/.exec(workflow)?.[1] ?? "";
expect(prRequiredNeeds, "could not read pr-required's needs list from ci.yml").not.toBe("");
expect(prRequiredNeeds).not.toMatch(/\bvisual-baseline\b/);
});

it("soft-fails only the pixel-comparison step, not the whole advisory job", () => {
// Job-level continue-on-error would also swallow setup / upload failures.
// Job keys in the captured block are indented four spaces; step keys are deeper.
expect(visualBaselineJob).not.toMatch(/^ {4}continue-on-error:\s*true\s*$/m);
expect(visualBaselineJob).toMatch(
/name: Chromium visual baselines\n\s+id: visual-comparison\n(?:\s+#.*\n)*\s+continue-on-error: true/,
);
});

it("reports pixel drift as a warning while preserving review artifacts", () => {
expect(visualBaselineJob).toContain("if: steps.visual-comparison.outcome == 'failure'");
expect(visualBaselineJob).toContain("scripts/classify-visual-baseline-outcome.mjs");
expect(visualBaselineJob).toContain("::warning title=Visual baseline drift::");
expect(visualBaselineJob).toContain("$GITHUB_STEP_SUMMARY");
expect(visualBaselineJob).toMatch(/name: Upload visual diffs\n\s+if: always\(\)/);
});
});

describe("Lighthouse budget routing", () => {
Expand Down
Loading
Loading