tests: harden the R2 baseline pipeline's failure paths (review findings)

- 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 <noreply@anthropic.com>

.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

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<string, string | boolean> {
@@ -71,8 +78,14 @@ async function main(): Promise<void> {
     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<void> {
         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<void> {
     };
     const { specFor } = buildSpecResolver(cwd);
 
-    const hashPng = async (s: R2Store, hash: string): Promise<PNG> => 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<PNG | null> => {
+        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<void> {
         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<void> {
         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<void> {
         `🗂️ **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);

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);

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;

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\`)`);

tests/tools/screenshots/promote.ts

@@ -149,15 +149,20 @@ async function main(): Promise<void> {
         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);

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;
         }