From 453df4fb9cb9aac1f5f5f9a8d011307cd0073c7d Mon Sep 17 00:00:00 2001 From: Istvan Matejcsok <119620946+matejcsok-ee@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:51:44 +0200 Subject: [PATCH] tests: harden the R2 baseline pipeline's failure paths (review findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - compare gate: refuse an INCOMPLETE cache, not just an empty one — a partial R2 fetch (continue-on-error step) previously misreported un-fetched baselines as ADDED and silently disabled their removed-detection. - post-discord: a missing report.json now posts "gate SKIPPED", never the false "✅ no screenshot drift" — a disabled pipeline must look disabled. - promote: hard-fail on a pre-migration (non-v2) manifest instead of warning; the warn path could commit manifest hashes never uploaded to R2. - changelog: tolerate per-image R2 failures (skip + loud note, like the old git-blob null-skip) instead of aborting the whole post; missing creds warn + exit 0 (notification-only workflow); git-show failures are loud errors, no longer mistaken for "predates the migration". - version guard: a manifest NEWER than the tooling throws everywhere instead of reading as a silent no-op. - manifest ordering: locale-independent code-unit comparator in writer + checker (localeCompare depends on host locale; writer=dev Mac, checker=CI). - r2-sync pull: byte-size pre-filter before hashing cached files. - wasm-build.yml: restore-keys on the baseline cache — manifest changes now restore the previous tree and download only the delta. Co-Authored-By: Claude Fable 5 --- .github/workflows/wasm-build.yml | 9 +++- tests/tools/screenshots/changelog.ts | 61 ++++++++++++++++++++----- tests/tools/screenshots/compare.ts | 19 +++++--- tests/tools/screenshots/gen-manifest.ts | 11 ++++- tests/tools/screenshots/post-discord.ts | 7 ++- tests/tools/screenshots/promote.ts | 17 ++++--- tests/tools/screenshots/r2-sync.ts | 15 ++++-- 7 files changed, 105 insertions(+), 34 deletions(-) diff --git a/.github/workflows/wasm-build.yml b/.github/workflows/wasm-build.yml index 52785de..65e5135 100644 --- a/.github/workflows/wasm-build.yml +++ b/.github/workflows/wasm-build.yml @@ -313,14 +313,19 @@ jobs: # Baseline screenshots live in a private R2 bucket, pinned by the committed # manifest; this cache keyed on the manifest hash makes most fetches a - # no-op (r2-sync hash-verifies every restored file, so a stale restore - # costs only the changed downloads). + # no-op. restore-keys makes a manifest change INCREMENTAL (restore the + # previous tree, download only the changed objects) instead of a full + # ~35MB re-download. A stale or partial restore is safe: r2-sync + # hash-verifies every file and deletes unlisted ones, and compare.ts + # refuses to gate against an incomplete cache. - name: Cache screenshot baselines if: inputs.run_tests uses: actions/cache@v4 with: path: tests/baseline-screenshots key: baselines-${{ hashFiles('tests/screenshot-manifest.json') }} + restore-keys: | + baselines- # continue-on-error: the screenshot pipeline is deliberately report-only # (see the report step) — an R2 outage must not fail the build. Without diff --git a/tests/tools/screenshots/changelog.ts b/tests/tools/screenshots/changelog.ts index 631c83b..ab846ed 100644 --- a/tests/tools/screenshots/changelog.ts +++ b/tests/tools/screenshots/changelog.ts @@ -31,13 +31,20 @@ function git(root: string, args: string[]): string { return execFileSync('git', ['-C', root, ...args], { encoding: 'utf8' }).trim(); } -/** The screenshot manifest as committed at `rev`, or null if absent/unparsable. */ +/** The screenshot manifest as committed at `rev`. Returns null ONLY when the + * file doesn't exist at that rev (pre-manifest era); any other git failure — + * shallow clone, bad revision, moved path — throws loudly instead of being + * mistaken for "predates the migration". Unparsable committed JSON also throws. */ function manifestAt(repoRoot: string, rev: string, repoPath: string): Manifest | null { + let raw: string; try { - return JSON.parse(execFileSync('git', ['-C', repoRoot, 'show', `${rev}:${repoPath}`], { encoding: 'utf8' })) as Manifest; - } catch { - return null; + raw = execFileSync('git', ['-C', repoRoot, 'show', `${rev}:${repoPath}`], { encoding: 'utf8' }); + } catch (e) { + const msg = ((e as { stderr?: Buffer }).stderr?.toString() ?? (e as Error).message).trim(); + if (/does not exist in|exists on disk, but not in/i.test(msg)) return null; + throw new Error(`git show ${rev}:${repoPath} failed: ${msg}`); } + return JSON.parse(raw) as Manifest; } function parseArgs(argv: string[]): Record { @@ -71,8 +78,14 @@ async function main(): Promise { const manifestPath = path.relative(repoRoot, path.join(cwd, MANIFEST_PATH)).split(path.sep).join('/'); const baseManifest = manifestAt(repoRoot, base, manifestPath); const headManifest = manifestAt(repoRoot, head, manifestPath); + // A NEWER manifest than this tooling understands is a loud error, never a skip. + for (const [label, m] of [['head', headManifest], ['base', baseManifest]] as const) { + if (m && m.version > MANIFEST_VERSION) { + throw new Error(`${label} manifest is version ${m.version}, newer than this tooling (expects ${MANIFEST_VERSION}) — update the checkout`); + } + } if (headManifest?.version !== MANIFEST_VERSION) { - console.log('[changelog] head manifest is not v2 — nothing to diff'); + console.log('[changelog] head manifest predates the R2 migration — nothing to diff'); return; } if (baseManifest?.version !== MANIFEST_VERSION) { @@ -93,10 +106,12 @@ async function main(): Promise { return; } + // Tolerate missing credentials (warn + exit 0): this is a notification-only + // workflow — turning every manifest push red during a key rotation is worse + // than one missed changelog post. The warning names the fix. const store = storeFromEnv(); if (!store) { - console.error(`[changelog] R2 credentials required to fetch baseline bytes: set ${missingEnv().join(', ')}`); - process.exitCode = 1; + console.warn(`[changelog] R2 credentials unset (${missingEnv().join(', ')}) — cannot fetch baseline bytes; skipping the changelog post`); return; } @@ -112,7 +127,18 @@ async function main(): Promise { }; const { specFor } = buildSpecResolver(cwd); - const hashPng = async (s: R2Store, hash: string): Promise => PNG.sync.read(await s.get(hash)); + // One unresolvable object must not abort the whole post — skip that image + // (with a loud note) and keep reporting the rest, like the old git-blob + // path null-skipped unresolvable blobs. + const skipped: string[] = []; + const hashPng = async (s: R2Store, hash: string, key: string): Promise => { + try { + return PNG.sync.read(await s.get(hash)); + } catch (e) { + skipped.push(`${key}: ${(e as Error).message}`); + return null; + } + }; // Save a single captioned image (added/removed) and record it in the report. const saveSingle = (img: PNG, key: string, status: 'added' | 'removed'): void => { const rel = path.join(DIFF_OUT_DIR, `${key.replace('/', '_')}.${status}.png`); @@ -120,11 +146,18 @@ async function main(): Promise { report[status].push({ name: key, image: rel }); }; - for (const key of added) saveSingle(await hashPng(store, headHashes.get(key)!), key, 'added'); - for (const key of removed) saveSingle(await hashPng(store, baseHashes.get(key)!), key, 'removed'); + for (const key of added) { + const img = await hashPng(store, headHashes.get(key)!, key); + if (img) saveSingle(img, key, 'added'); + } + for (const key of removed) { + const img = await hashPng(store, baseHashes.get(key)!, key); + if (img) saveSingle(img, key, 'removed'); + } for (const key of changed) { - const oldImg = await hashPng(store, baseHashes.get(key)!); - const newImg = await hashPng(store, headHashes.get(key)!); + const oldImg = await hashPng(store, baseHashes.get(key)!, key); + const newImg = await hashPng(store, headHashes.get(key)!, key); + if (!oldImg || !newImg) continue; const { result, heatmap, triptych } = comparePair(oldImg, newImg, key, floorFor(key)); const triptychRel = path.join(DIFF_OUT_DIR, `${key.replace('/', '_')}.triptych.png`); const heatmapRel = path.join(DIFF_OUT_DIR, `${key.replace('/', '_')}.heatmap.png`); @@ -133,6 +166,9 @@ async function main(): Promise { report.changed.push({ ...result, triptych: triptychRel, heatmap: heatmapRel }); } report.changed.sort((a, b) => b.changedRatio - a.changedRatio); + if (skipped.length) { + console.warn(`[changelog] skipped ${skipped.length} unresolvable image(s):\n ${skipped.join('\n ')}`); + } const sha7 = (process.env.GITHUB_SHA || head).slice(0, 7); let subject = ''; @@ -143,6 +179,7 @@ async function main(): Promise { `🗂️ **Baseline changelog** · \`${sha7}\`` + (subject ? `\n> ${subject}` : '') + `\n${report.changed.length} changed, ${report.added.length} added, ${report.removed.length} removed` + + (skipped.length ? `\n⚠️ ${skipped.length} image(s) unresolvable in R2 — see the workflow log` : '') + (report.removed.length ? '\n➖ REMOVED: ' + report.removed.map((r) => `\`${r.name}\``).join(', ') : ''); const { files, notes } = buildAttachments(cwd, report); diff --git a/tests/tools/screenshots/compare.ts b/tests/tools/screenshots/compare.ts index 9b406cb..e85ae71 100644 --- a/tests/tools/screenshots/compare.ts +++ b/tests/tools/screenshots/compare.ts @@ -229,13 +229,18 @@ function main(): void { } const root = process.cwd(); // Baselines are a local cache fetched from R2 (`npm run screenshots:fetch`). - // When the manifest expects screenshots but the cache is empty — a secretless - // CI caller where the fetch step skipped, or a dev who hasn't fetched — skip - // the gate instead of misreporting every baseline as removed. No report.json - // is written; post-discord tolerates its absence. - const expected = loadManifest(root)?.screenshots.length ?? 0; - if (expected > 0 && listEngineKeys(path.join(root, BASELINE_ROOT)).length === 0) { - console.log('[compare] baseline cache is empty — run `npm run screenshots:fetch` (needs R2 credentials); skipping'); + // The gate only runs against a COMPLETE cache: a partial one (an R2 blip on + // a few objects — the CI fetch step is continue-on-error) would misreport + // the un-fetched baselines' renders as ADDED and silently disable removed- + // detection for them. Fully missing = fetch skipped (no credentials) or a + // dev who hasn't fetched. Either way skip without writing report.json — + // post-discord posts a distinct "gate SKIPPED" line when it's absent. + const gateManifest = loadManifest(root); + const wanted = (gateManifest?.screenshots ?? []).filter((e) => !isIgnored(`${e.engine}/${e.name}`)); + const missing = wanted.filter((e) => !fs.existsSync(path.join(root, BASELINE_ROOT, e.engine, e.name))); + if (wanted.length && missing.length) { + const what = missing.length === wanted.length ? 'empty' : `INCOMPLETE (${missing.length}/${wanted.length} missing — partial fetch?)`; + console.log(`[compare] baseline cache is ${what} — run \`npm run screenshots:fetch\` (needs R2 credentials); skipping`); return; } const report = classify(root, (args.sha as string) || process.env.GITHUB_SHA || null); diff --git a/tests/tools/screenshots/gen-manifest.ts b/tests/tools/screenshots/gen-manifest.ts index aff3546..a26cec2 100644 --- a/tests/tools/screenshots/gen-manifest.ts +++ b/tests/tools/screenshots/gen-manifest.ts @@ -39,6 +39,13 @@ import { } from './config'; import { hashFile, missingEnv, storeFromEnv } from './r2-store'; +/** Locale-independent (code-unit) ordering — the writer runs on dev machines, + * the --check gate on CI, and localeCompare's result depends on the host locale. */ +function compareEntries(a: { engine: string; name: string }, b: { engine: string; name: string }): number { + if (a.engine !== b.engine) return a.engine < b.engine ? -1 : 1; + return a.name < b.name ? -1 : a.name > b.name ? 1 : 0; +} + const NOTE = 'Baselines live in R2, content-addressed by sha256 — the local baseline-screenshots/ tree is a cache ' + '(`npm run screenshots:fetch`). Regenerated by `npm run screenshots:promote`; never edit hashes by hand. ' + @@ -62,7 +69,7 @@ export function manifestJson(root: string): string { height: png.height, }; }) - .sort((a, b) => a.engine.localeCompare(b.engine) || a.name.localeCompare(b.name)); + .sort(compareEntries); const manifest: Manifest & { _note: string } = { _note: NOTE, @@ -108,7 +115,7 @@ export function checkManifest(root: string): string[] { if (IGNORE_SCREENSHOTS.has(e.name)) problems.push(`${id}: is in IGNORE_SCREENSHOTS and must not be listed`); if (seen.has(id)) problems.push(`${id}: duplicate entry`); seen.add(id); - if (prev && (prev.engine.localeCompare(e.engine) || prev.name.localeCompare(e.name)) > 0) { + if (prev && compareEntries(prev, e) > 0) { problems.push(`${id}: not sorted (engine, then name)`); } prev = e; diff --git a/tests/tools/screenshots/post-discord.ts b/tests/tools/screenshots/post-discord.ts index 3582fe6..611f220 100644 --- a/tests/tools/screenshots/post-discord.ts +++ b/tests/tools/screenshots/post-discord.ts @@ -134,7 +134,12 @@ function buildHeader(report: Report | null, perfBlock: string, meta: { sha?: str }; lines.push(''); - if (!report || (!report.changed.length && !report.added.length && !report.removed.length)) { + if (!report) { + // No report.json = the compare gate never ran (baseline fetch skipped or + // incomplete). Saying "no drift" here would make a disabled pipeline + // indistinguishable from a healthy one. + lines.push('⚠️ screenshot gate SKIPPED — no baseline comparison ran (R2 fetch skipped/failed; check CI_SCREENSHOTS_S3_* secrets and the fetch step log)'); + } else if (!report.changed.length && !report.added.length && !report.removed.length) { lines.push('✅ no screenshot drift'); } else if (report.driftLikely) { lines.push(`⚠️ **${report.changed.length} screenshots changed broadly** — looks like host env drift → re-promote (\`npm run screenshots:promote\`)`); diff --git a/tests/tools/screenshots/promote.ts b/tests/tools/screenshots/promote.ts index d731043..f6d0a52 100644 --- a/tests/tools/screenshots/promote.ts +++ b/tests/tools/screenshots/promote.ts @@ -149,15 +149,20 @@ async function main(): Promise { process.exitCode = 2; return; } + // Refuse a pre-migration checkout rather than warn: buildPlan would mark + // most renders "unchanged" (so never uploaded) while writeManifest pins + // their hashes anyway — committing a manifest that references objects R2 + // doesn't have. Sync the checkout to a post-migration revision first. + if (!loadManifestV2(root)) { + console.error(`[promote] ${MANIFEST_PATH} is not the R2-backed v2 format — refusing to promote on a pre-migration checkout`); + process.exitCode = 2; + return; + } // The local tree is a cache — sync it to the committed manifest so the plan // diffs against exactly what the manifest pins (a stale/absent cache would // otherwise misreport adds/updates). - if (loadManifestV2(root)) { - const { downloaded, cached, deleted } = await pullBaselines(root, store); - console.log(`[promote] cache synced: downloaded=${downloaded} cached=${cached} deleted=${deleted}`); - } else { - console.warn(`[promote] ${MANIFEST_PATH} is not v2 — skipping cache sync (pre-migration tree?)`); - } + const { downloaded, cached, deleted } = await pullBaselines(root, store); + console.log(`[promote] cache synced: downloaded=${downloaded} cached=${cached} deleted=${deleted}`); const renderDir = args.from ? (args.from as string) : downloadRun(args.run as string, args.repo as string); const manifest = loadManifest(root); diff --git a/tests/tools/screenshots/r2-sync.ts b/tests/tools/screenshots/r2-sync.ts index cccfe38..d1a63ed 100644 --- a/tests/tools/screenshots/r2-sync.ts +++ b/tests/tools/screenshots/r2-sync.ts @@ -22,16 +22,22 @@ import { R2Store, hashFile, missingEnv, storeFromEnv } from './r2-store'; const CONCURRENCY = 16; -/** Parse the committed manifest if it is the R2-backed v2 format, else null. */ +/** Parse the committed manifest if it is the R2-backed v2 format; null for a + * pre-migration (v1/absent/unparsable) manifest. A NEWER version throws — old + * tooling silently no-oping on a future format would disable the whole gate. */ export function loadManifestV2(root: string): Manifest | null { const p = path.join(root, MANIFEST_PATH); if (!fs.existsSync(p)) return null; + let m: Manifest; try { - const m = JSON.parse(fs.readFileSync(p, 'utf8')) as Manifest; - return m.version === MANIFEST_VERSION ? m : null; + m = JSON.parse(fs.readFileSync(p, 'utf8')) as Manifest; } catch { return null; } + if (m.version > MANIFEST_VERSION) { + throw new Error(`${MANIFEST_PATH} is version ${m.version} but this tooling expects ${MANIFEST_VERSION} — update your checkout`); + } + return m.version === MANIFEST_VERSION ? m : null; } /** Run `fn` over `items` with at most `limit` in flight. */ @@ -65,7 +71,8 @@ export async function pullBaselines( const errors: string[] = []; await pool([...wanted.values()], CONCURRENCY, async (e) => { const dest = path.join(base, e.engine, e.name); - if (fs.existsSync(dest) && hashFile(dest) === e.sha256) { + // Size is a cheap pre-filter: only pay the full read + hash when it can match. + if (fs.existsSync(dest) && fs.statSync(dest).size === e.bytes && hashFile(dest) === e.sha256) { cached++; return; }