From e165c856f84e6adb52b70f8b1b0c7dbf30b0e25e Mon Sep 17 00:00:00 2001
From: "mykola.gervasyuk" <gervasyuk@gmail.com>
Date: Fri, 4 Sep 2026 09:36:44 +0300
Subject: [PATCH 1/3] perf: sign a run once at ingest instead of on every
 review
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Measured on production: opening "Approve variations" spends ~4.5s in
matchingSiblings before a single thumbnail is requested. For a screen with
34 locales that call fetches 70 full-size screenshots back out of S3 and
decodes them, eight at a time. The concurrency bound added with the worker
pool was chosen to protect memory and CPU; on S3 the work is network-bound,
so eight is simply the number of round trips it waits on.

None of that has to happen at review time. The diff already decodes both
screenshots at ingest, so the signature is now produced by that same pass
and stored on the run. Grouping a screen becomes a column read.

- computePixelmatchDiff optionally returns the signature, computed from the
  images it has already decoded and ignore-masked. Both paths call one
  signatureOfDecoded, and a test pins the fused answer to be identical to
  the standalone job's — if they drifted, stored signatures would silently
  stop matching computed ones and variations would stop grouping.
- It is asked for on every comparison rather than only when the project has
  bulk approve switched on, so turning the flag on later does not leave a
  build's runs unsigned.
- saveDiffResult writes it, and always overwrites: recomputing a diff after
  the reviewer edits the ignore areas must not leave the old signature
  describing a change that no longer exists.
- Runs with nothing stored — ingested before this, or compared by something
  other than pixelmatch — fall back to the existing worker-pool path, so no
  backfill is needed and old builds keep working.

The column is nullable with no default, so the migration is catalogue-only:
no table rewrite, no long lock on a TestRun table holding a hundred builds.
---
 .../migration.sql                             |  6 ++
 prisma/schema.prisma                          |  9 +-
 src/_data_/index.ts                           |  1 +
 .../libs/pixelmatch/pixelmatch.core.spec.ts   | 87 +++++++++++++++++++
 .../libs/pixelmatch/pixelmatch.core.ts        | 31 ++++++-
 .../pixelmatch/pixelmatch.service.spec.ts     | 66 ++++++++++++++
 .../libs/pixelmatch/pixelmatch.service.ts     |  7 ++
 src/compare/libs/pixelmatch/signature.core.ts | 24 +++--
 src/test-runs/diffResult.ts                   |  8 ++
 src/test-runs/test-runs.service.spec.ts       | 80 +++++++++++++++++
 src/test-runs/test-runs.service.ts            | 37 +++++++-
 11 files changed, 345 insertions(+), 11 deletions(-)
 create mode 100644 prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql
 create mode 100644 src/compare/libs/pixelmatch/pixelmatch.core.spec.ts

diff --git a/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql
new file mode 100644
index 00000000..62cbaa8d
--- /dev/null
+++ b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql
@@ -0,0 +1,6 @@
+-- AlterTable
+-- Nullable with no default, so this is a catalogue-only change on Postgres 11+:
+-- no table rewrite and no long lock, which matters on a TestRun table holding
+-- a hundred builds' worth of runs. Existing rows keep NULL and fall back to
+-- computing the signature on demand.
+ALTER TABLE "TestRun" ADD COLUMN "changeSignature" TEXT;
diff --git a/prisma/schema.prisma b/prisma/schema.prisma
index 4835aeae..104dd8aa 100644
--- a/prisma/schema.prisma
+++ b/prisma/schema.prisma
@@ -1,5 +1,5 @@
 generator client {
-  provider = "prisma-client-js"
+  provider      = "prisma-client-js"
   binaryTargets = ["native", "debian-openssl-3.0.x", "linux-arm64-openssl-3.0.x"]
 }
 
@@ -72,6 +72,13 @@ model TestRun {
   ignoreAreas           String         @default("[]")
   tempIgnoreAreas       String         @default("[]")
   vlmDescription        String?
+  // Position-independent colour signature of this run's change, as JSON, so the
+  // variations dialog can group a screen's locales without fetching and
+  // decoding every sibling's screenshots at review time. Written once, next to
+  // the diff that already decoded them. Null on runs that predate this, that
+  // have no diff, or that were compared by something other than pixelmatch —
+  // those fall back to computing it on demand.
+  changeSignature       String?
   baseline              Baseline?
   build                 Build          @relation(fields: [buildId], references: [id])
   project               Project?       @relation(fields: [projectId], references: [id])
diff --git a/src/_data_/index.ts b/src/_data_/index.ts
index fcbb4df9..7fc3bc27 100644
--- a/src/_data_/index.ts
+++ b/src/_data_/index.ts
@@ -99,6 +99,7 @@ export const generateTestRun = (testRun?: Partial<TestRun>): TestRun => {
     branchName: 'develop',
     merge: false,
     vlmDescription: null,
+    changeSignature: null,
     ...testRun,
   };
 };
diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts
new file mode 100644
index 00000000..5e9e47c9
--- /dev/null
+++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts
@@ -0,0 +1,87 @@
+import { PNG } from 'pngjs';
+import { computePixelmatchDiff } from './pixelmatch.core';
+import { computeChangeSignature } from './signature.core';
+
+const WIDTH = 60;
+const HEIGHT = 60;
+
+const png = (paint: (set: (x: number, y: number, rgb: [number, number, number]) => void) => void): Buffer => {
+  const image = new PNG({ width: WIDTH, height: HEIGHT });
+  for (let i = 0; i < WIDTH * HEIGHT; i++) {
+    image.data[i * 4] = 255;
+    image.data[i * 4 + 1] = 255;
+    image.data[i * 4 + 2] = 255;
+    image.data[i * 4 + 3] = 255;
+  }
+  paint((x, y, [r, g, b]) => {
+    const index = (y * WIDTH + x) * 4;
+    image.data[index] = r;
+    image.data[index + 1] = g;
+    image.data[index + 2] = b;
+  });
+  return PNG.sync.write(image);
+};
+
+const block = (left: number, top: number, rgb: [number, number, number]): Buffer =>
+  png((set) => {
+    for (let y = top; y < top + 12; y++) {
+      for (let x = left; x < left + 12; x++) {
+        set(x, y, rgb);
+      }
+    }
+  });
+
+const blank = png(() => undefined);
+const RED: [number, number, number] = [255, 0, 0];
+
+const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean) =>
+  computePixelmatchDiff({
+    kind: 'diff',
+    baseline,
+    image,
+    ignoreAreas: [],
+    threshold: 0.1,
+    includeAA: false,
+    allowDiffDimensions: false,
+    diffTolerancePercent: 0,
+    saveDiff: false,
+    withSignature,
+  });
+
+describe('computePixelmatchDiff with a signature', () => {
+  // The whole point of computing it here is to reuse the decode the diff
+  // already paid for. If the fused version answered differently from the
+  // standalone one, stored signatures would stop matching computed ones and
+  // variations would silently stop grouping.
+  it('gives the same signature the standalone job would', () => {
+    const image = block(10, 10, RED);
+
+    const fused = diffJob(blank, image, true).signature;
+    const standalone = computeChangeSignature({
+      kind: 'signature',
+      baseline: blank,
+      image,
+      ignoreAreas: [],
+      threshold: 0.1,
+      includeAA: false,
+    }).signature;
+
+    expect(fused).toEqual(standalone);
+    expect(fused).not.toBeNull();
+  });
+
+  it('leaves the signature out when it was not asked for', () => {
+    expect(diffJob(blank, block(10, 10, RED), false).signature).toBeUndefined();
+  });
+
+  it('has no signature when the screenshots are identical', () => {
+    expect(diffJob(blank, blank, true).signature).toBeUndefined();
+  });
+
+  it('still reports the diff it was asked for', () => {
+    const result = diffJob(blank, block(10, 10, RED), true);
+
+    expect(result.equal).toBe(false);
+    expect(result.pixelMisMatchCount).toBe(144);
+  });
+});
diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts
index 47bbc71e..9dc12968 100644
--- a/src/compare/libs/pixelmatch/pixelmatch.core.ts
+++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts
@@ -2,6 +2,7 @@ import { PNG } from 'pngjs';
 import Pixelmatch from 'pixelmatch';
 import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto';
 import { applyIgnoreAreas, scaleImageToSize } from '../../utils';
+import { signatureOfDecoded } from './signature.core';
 
 /**
  * CPU-bound part of the pixelmatch comparison, extracted so it can run inside
@@ -20,6 +21,13 @@ export interface PixelmatchJobInput {
   allowDiffDimensions: boolean;
   diffTolerancePercent: number;
   saveDiff: boolean;
+  /**
+   * Also produce the change signature the variations dialog matches on. Free
+   * here next to the diff — the images are decoded already — and computed once
+   * at ingest so reviewing never has to fetch and decode a build's screenshots
+   * again just to group them.
+   */
+  withSignature?: boolean;
 }
 
 export interface PixelmatchJobOutput {
@@ -28,6 +36,9 @@ export interface PixelmatchJobOutput {
   pixelMisMatchCount?: number;
   diffPercent?: number;
   diffBuffer?: Buffer | Uint8Array;
+  // absent when not asked for, when the screenshots are identical, or when
+  // their dimensions differ and there is nothing meaningful to sign
+  signature?: number[];
 }
 
 // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer.
@@ -74,5 +85,23 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO
     diffBuffer = PNG.sync.write(diff);
   }
 
-  return { equal: false, isSameDimension, pixelMisMatchCount, diffPercent, diffBuffer };
+  // Same source images, same ignore areas, same threshold as the standalone
+  // signature job — sharing signatureOfDecoded is what keeps the two answers
+  // identical, so a stored signature still matches one computed on the fly.
+  const signature =
+    input.withSignature && isSameDimension
+      ? signatureOfDecoded(baselineIgnored, imageIgnored, {
+          threshold: input.threshold,
+          includeAA: input.includeAA,
+        })
+      : null;
+
+  return {
+    equal: false,
+    isSameDimension,
+    pixelMisMatchCount,
+    diffPercent,
+    diffBuffer,
+    ...(signature ? { signature } : {}),
+  };
 }
diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
index 1696b97d..2ad99a9f 100644
--- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
+++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
@@ -257,3 +257,69 @@ describe('getDiff', () => {
     });
   });
 });
+
+describe('change signature', () => {
+  // Correctness of the signature itself is pinned in pixelmatch.core.spec
+  // against the real pixelmatch; Pixelmatch is mocked in this file, so what is
+  // worth testing here is only the plumbing — that the service asks for one and
+  // carries what comes back into the result.
+  const initWithPool = async (output: Record<string, unknown>) => {
+    const run = jest.fn().mockResolvedValue(output);
+    const module: TestingModule = await Test.createTestingModule({
+      providers: [
+        PixelmatchService,
+        { provide: DiffWorkerPool, useValue: { run } },
+        {
+          provide: StaticService,
+          useValue: {
+            getImageBuffer: jest.fn().mockResolvedValue(Buffer.from('png')),
+            saveImage: jest.fn(),
+            deleteImage: jest.fn(),
+          },
+        },
+      ],
+    }).compile();
+    return { service: module.get<PixelmatchService>(PixelmatchService), run };
+  };
+
+  const compare = (service: PixelmatchService) =>
+    service.getDiff(
+      {
+        baseline: 'baseline.png',
+        image: 'image.png',
+        ignoreAreas: [],
+        diffTollerancePercent: 0,
+        saveDiffAsFile: false,
+      },
+      DEFAULT_CONFIG
+    );
+
+  // Asked for on every comparison, not only when the project has bulk approve
+  // switched on: computing it lazily would leave every run ingested before the
+  // flag was turned on without one.
+  it('asks for a signature and carries it into the result', async () => {
+    const { service, run } = await initWithPool({
+      equal: false,
+      isSameDimension: true,
+      pixelMisMatchCount: 10,
+      diffPercent: 5,
+      signature: [0.25, 0.75],
+    });
+
+    const result = await compare(service);
+
+    expect(run).toHaveBeenCalledWith(expect.objectContaining({ kind: 'diff', withSignature: true }));
+    expect(result.changeSignature).toEqual([0.25, 0.75]);
+  });
+
+  it('leaves it out when the comparison produced none', async () => {
+    const { service } = await initWithPool({
+      equal: false,
+      isSameDimension: true,
+      pixelMisMatchCount: 10,
+      diffPercent: 5,
+    });
+
+    expect((await compare(service)).changeSignature).toBeUndefined();
+  });
+});
diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts
index 0cb9c800..a5092f79 100644
--- a/src/compare/libs/pixelmatch/pixelmatch.service.ts
+++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts
@@ -45,6 +45,12 @@ export class PixelmatchService implements ImageComparator {
       allowDiffDimensions: config.allowDiffDimensions,
       diffTolerancePercent: data.diffTollerancePercent,
       saveDiff: data.saveDiffAsFile,
+      // Asked for on every comparison, not only when the project has bulk
+      // approve of variations switched on: the pass costs little beside the
+      // full-size diff that has already been paid for, and computing it lazily
+      // would leave every run ingested before the flag was turned on without
+      // one — which is exactly the build someone then tries to review.
+      withSignature: true,
     });
 
     if (output.equal) {
@@ -59,6 +65,7 @@ export class PixelmatchService implements ImageComparator {
       isSameDimension: output.isSameDimension,
       pixelMisMatchCount: output.pixelMisMatchCount,
       diffPercent: output.diffPercent,
+      ...(output.signature ? { changeSignature: output.signature } : {}),
     };
 
     if (result.diffPercent > data.diffTollerancePercent) {
diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts
index 9909354c..a2701fdb 100644
--- a/src/compare/libs/pixelmatch/signature.core.ts
+++ b/src/compare/libs/pixelmatch/signature.core.ts
@@ -106,17 +106,31 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu
   applyIgnoreAreas(baselineImage, input.ignoreAreas);
   applyIgnoreAreas(checkpointImage, input.ignoreAreas);
 
+  return { signature: signatureOfDecoded(baselineImage, checkpointImage, input) };
+}
+
+/**
+ * The signature of a pair that is already decoded, of equal size, and with its
+ * ignore areas blanked. Split out so the diff can produce a signature from the
+ * images it has just decoded rather than paying for a second decode — the two
+ * must not drift apart, so there is only ever one implementation.
+ */
+export function signatureOfDecoded(
+  baselineImage: RawImage,
+  checkpointImage: RawImage,
+  options: { threshold: number; includeAA: boolean }
+): number[] | null {
   const baseline = downscale(baselineImage, SIGNATURE_MAX_DIMENSION);
   const image = downscale(checkpointImage, SIGNATURE_MAX_DIMENSION);
   const { width, height } = baseline;
   const mask = new PNG({ width, height });
   const changedPixels = Pixelmatch(baseline.data, image.data, mask.data, width, height, {
-    threshold: input.threshold,
-    includeAA: input.includeAA,
+    threshold: options.threshold,
+    includeAA: options.includeAA,
     diffMask: true,
   });
   if (changedPixels === 0) {
-    return { signature: null };
+    return null;
   }
 
   const bucketSize = 256 / COLOR_BUCKETS_PER_CHANNEL;
@@ -133,9 +147,9 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu
 
   const total = histogram.reduce((sum, value) => sum + value, 0);
   if (total === 0) {
-    return { signature: null };
+    return null;
   }
-  return { signature: histogram.map((value) => value / total) };
+  return histogram.map((value) => value / total);
 }
 
 function cosineSimilarity(a: number[], b: number[]): number {
diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts
index b4219595..239a092a 100644
--- a/src/test-runs/diffResult.ts
+++ b/src/test-runs/diffResult.ts
@@ -12,4 +12,12 @@ export interface DiffResult {
    * Can be displayed as bullet points in UI
    */
   vlmDescription?: string;
+  /**
+   * Position-independent colour signature of the change, produced by the same
+   * pass that produced the diff. Stored on the run so the variations dialog can
+   * group a screen's locales without decoding their screenshots all over again.
+   * Absent when nothing changed, when the dimensions differ, or when the
+   * comparison was not pixelmatch.
+   */
+  changeSignature?: number[];
 }
diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts
index 57d2da60..14571c3d 100644
--- a/src/test-runs/test-runs.service.spec.ts
+++ b/src/test-runs/test-runs.service.spec.ts
@@ -350,11 +350,47 @@ describe('TestRunsService', () => {
           pixelMisMatchCount: null,
           diffPercent: null,
           vlmDescription: null,
+          changeSignature: null,
         },
       });
       expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun);
     });
 
+    // Written here, next to the diff that decoded the screenshots anyway, so
+    // the variations dialog never has to decode them again.
+    it('stores the change signature the comparison produced', async () => {
+      const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun);
+      service = await initService({ testRunUpdateMock });
+
+      await service.saveDiffResult('some id', {
+        status: TestStatus.unresolved,
+        diffName: 'diff image name',
+        pixelMisMatchCount: 11,
+        diffPercent: 22,
+        isSameDimension: true,
+        changeSignature: [0.25, 0.75],
+      });
+
+      expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({
+        changeSignature: JSON.stringify([0.25, 0.75]),
+      });
+    });
+
+    it('stores no signature when the comparison produced none', async () => {
+      const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun);
+      service = await initService({ testRunUpdateMock });
+
+      await service.saveDiffResult('some id', {
+        status: TestStatus.ok,
+        diffName: null,
+        pixelMisMatchCount: 0,
+        diffPercent: 0,
+        isSameDimension: true,
+      });
+
+      expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ changeSignature: null });
+    });
+
     it('with results', async () => {
       const diff: DiffResult = {
         status: TestStatus.unresolved,
@@ -382,6 +418,7 @@ describe('TestRunsService', () => {
           pixelMisMatchCount: diff.pixelMisMatchCount,
           diffPercent: diff.diffPercent,
           vlmDescription: diff.vlmDescription,
+          changeSignature: null,
         },
       });
       expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun);
@@ -774,6 +811,49 @@ describe('TestRunsService', () => {
       ]);
     });
 
+    // The whole point of storing it: a build's screenshots are fetched and
+    // decoded once at ingest, never again to group a screen at review time.
+    it('groups from the signatures stored at ingest, without decoding anything', async () => {
+      const signed = (id: string, signature: number[], overrides = {}) =>
+        sibling(id, { changeSignature: JSON.stringify(signature), ...overrides });
+      const testRun = signed('target', SAME_PALETTE, { customTags: '' });
+      const matching = signed('locale-a', SAME_PALETTE);
+      const different = signed('locale-c', OTHER_PALETTE);
+
+      const built = await initMatchingService({
+        testRun,
+        siblings: [matching, different],
+        signatureOf: () => {
+          throw new Error('should not be computing a signature');
+        },
+      });
+
+      const result = await built.service.getMatchingVariations(testRun.id);
+
+      expect(built.compareGetChangeSignatureMock).not.toHaveBeenCalled();
+      expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]);
+      expect(result.skipped.map((item) => item.reason)).toEqual(['different change pattern']);
+    });
+
+    // builds ingested before the column existed, and runs compared by something
+    // other than pixelmatch, still have to group
+    it('falls back to computing for a run with nothing stored', async () => {
+      const testRun = sibling('target', { customTags: '', changeSignature: JSON.stringify(SAME_PALETTE) });
+      const unsigned = sibling('locale-a');
+
+      const built = await initMatchingService({
+        testRun,
+        siblings: [unsigned],
+        signatureOf: () => SAME_PALETTE,
+      });
+
+      const result = await built.service.getMatchingVariations(testRun.id);
+
+      const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString());
+      expect(asked).toEqual([unsigned.imageName]);
+      expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, unsigned.id]);
+    });
+
     it('skips a far larger change without paying for its signature', async () => {
       const testRun = sibling('target', { customTags: '' });
       const bigger = sibling('locale-b', { diffPercent: 30 });
diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts
index 2104e37f..d97166a7 100644
--- a/src/test-runs/test-runs.service.ts
+++ b/src/test-runs/test-runs.service.ts
@@ -292,13 +292,21 @@ export class TestRunsService {
   /**
    * Position-independent color signature of the change between a test run's
    * baseline and image. Null when there is no baseline, dimensions differ, or
-   * nothing changed. The bytes go to the worker pool undecoded — decoding is
-   * the expensive part and must not happen on the event loop.
+   * nothing changed.
    *
-   * Memoized: a reviewer who reopens the variations dialog, or steps back to a
-   * screen already looked at, would otherwise pay for the same decodes again.
+   * Normally this was written at ingest, beside the diff that had already
+   * decoded both screenshots, and grouping a screen costs nothing but the row
+   * it is read from. The rest of this is the fallback for runs that carry no
+   * stored signature — ingested before the column existed, or compared by
+   * something other than pixelmatch: their bytes go to the worker pool
+   * undecoded, and the result is memoized so reopening the dialog on an old
+   * build does not pay for the same decodes twice.
    */
   private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise<number[] | null> {
+    const stored = parseStoredSignature(testRun.changeSignature, this.logger);
+    if (stored) {
+      return stored;
+    }
     if (!testRun.baselineName) {
       return null;
     }
@@ -368,6 +376,10 @@ export class TestRunsService {
           diffPercent: diffResult && diffResult.diffPercent,
           status: diffResult ? diffResult.status : TestStatus.new,
           vlmDescription: diffResult && diffResult?.vlmDescription,
+          // Always written, never merged: a recomputed diff — after the
+          // reviewer edits the ignore areas, say — must not leave the previous
+          // signature behind describing a change that no longer exists.
+          changeSignature: diffResult?.changeSignature ? JSON.stringify(diffResult.changeSignature) : null,
         },
       })
       .then((testRun) => {
@@ -676,6 +688,23 @@ async function mapWithConcurrency<T, R>(items: T[], limit: number, task: (item:
   return results;
 }
 
+/**
+ * The signature stored on a run, or null when there is none to read. Bad JSON
+ * is not worth failing a review over: the caller falls back to computing it.
+ */
+function parseStoredSignature(stored: string | null | undefined, logger: Logger): number[] | null {
+  if (!stored) {
+    return null;
+  }
+  try {
+    const parsed = JSON.parse(stored);
+    return Array.isArray(parsed) && parsed.length > 0 ? parsed : null;
+  } catch (error) {
+    logger.warn(`Ignoring unreadable stored change signature: ${error}`);
+    return null;
+  }
+}
+
 // Same palette but a much larger/smaller change area signals a different or
 // additional change (an extra element changed too), not just per-locale text
 // reflow — which stays well under this ratio. Such variations go to manual review.

From 1b637cb2bf171d583f188abf48b4a5e72f6fe20c Mon Sep 17 00:00:00 2001
From: "mykola.gervasyuk" <gervasyuk@gmail.com>
Date: Fri, 4 Sep 2026 11:21:02 +0300
Subject: [PATCH 2/3] fix: discard a stored signature when the comparison
 settings move on
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A signature only means anything under the threshold it was computed with.
The in-memory memo has always keyed on threshold and includeAA; when the
same value started being persisted, that guard was not carried over.

So a project whose imageComparisonConfig is edited after a build was
ingested could compare a stored signature, taken under the old settings,
against a sibling's computed fresh under the new ones. Nothing breaks
loudly: variations simply stop grouping as well as they did, and the
reviewer has no way to tell why.

The settings now travel with the signature, and a stored one whose settings
no longer match the project's is discarded — which falls back to computing
it, exactly as for a run that never had one. The optimisation now knows what
it was computed under.

Found by CodeRabbit on #376.
---
 .../pixelmatch/pixelmatch.service.spec.ts     |  8 ++++-
 .../libs/pixelmatch/pixelmatch.service.ts     | 10 +++++-
 src/test-runs/diffResult.ts                   | 23 +++++++++---
 src/test-runs/test-runs.service.spec.ts       | 36 ++++++++++++++++---
 src/test-runs/test-runs.service.ts            | 28 +++++++++++----
 5 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
index 2ad99a9f..2a659f40 100644
--- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
+++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts
@@ -309,7 +309,13 @@ describe('change signature', () => {
     const result = await compare(service);
 
     expect(run).toHaveBeenCalledWith(expect.objectContaining({ kind: 'diff', withSignature: true }));
-    expect(result.changeSignature).toEqual([0.25, 0.75]);
+    // stamped with the settings it was computed under, so a stored one can be
+    // discarded when the project's config moves on
+    expect(result.changeSignature).toEqual({
+      threshold: DEFAULT_CONFIG.threshold,
+      includeAA: DEFAULT_CONFIG.ignoreAntialiasing,
+      signature: [0.25, 0.75],
+    });
   });
 
   it('leaves it out when the comparison produced none', async () => {
diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts
index a5092f79..55b184a8 100644
--- a/src/compare/libs/pixelmatch/pixelmatch.service.ts
+++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts
@@ -65,7 +65,15 @@ export class PixelmatchService implements ImageComparator {
       isSameDimension: output.isSameDimension,
       pixelMisMatchCount: output.pixelMisMatchCount,
       diffPercent: output.diffPercent,
-      ...(output.signature ? { changeSignature: output.signature } : {}),
+      ...(output.signature
+        ? {
+            changeSignature: {
+              threshold: config.threshold,
+              includeAA: config.ignoreAntialiasing,
+              signature: output.signature,
+            },
+          }
+        : {}),
     };
 
     if (result.diffPercent > data.diffTollerancePercent) {
diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts
index 239a092a..c9bfce15 100644
--- a/src/test-runs/diffResult.ts
+++ b/src/test-runs/diffResult.ts
@@ -1,5 +1,17 @@
 import { TestStatus } from '@prisma/client';
 
+/**
+ * A signature and the comparison settings it was computed under. A signature
+ * only means anything under its own threshold, so comparing one taken at a
+ * different setting quietly makes grouping worse — the settings travel with it
+ * so a stored one can be discarded when the project's config moves on.
+ */
+export interface StampedSignature {
+  threshold: number;
+  includeAA: boolean;
+  signature: number[];
+}
+
 export interface DiffResult {
   status: TestStatus;
   diffName: string;
@@ -14,10 +26,11 @@ export interface DiffResult {
   vlmDescription?: string;
   /**
    * Position-independent colour signature of the change, produced by the same
-   * pass that produced the diff. Stored on the run so the variations dialog can
-   * group a screen's locales without decoding their screenshots all over again.
-   * Absent when nothing changed, when the dimensions differ, or when the
-   * comparison was not pixelmatch.
+   * pass that produced the diff, carrying the settings it was produced under.
+   * Stored on the run so the variations dialog can group a screen's locales
+   * without decoding their screenshots all over again. Absent when nothing
+   * changed, when the dimensions differ, or when the comparison was not
+   * pixelmatch.
    */
-  changeSignature?: number[];
+  changeSignature?: StampedSignature;
 }
diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts
index 14571c3d..4ed710e3 100644
--- a/src/test-runs/test-runs.service.spec.ts
+++ b/src/test-runs/test-runs.service.spec.ts
@@ -368,11 +368,11 @@ describe('TestRunsService', () => {
         pixelMisMatchCount: 11,
         diffPercent: 22,
         isSameDimension: true,
-        changeSignature: [0.25, 0.75],
+        changeSignature: { threshold: 0.1, includeAA: true, signature: [0.25, 0.75] },
       });
 
       expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({
-        changeSignature: JSON.stringify([0.25, 0.75]),
+        changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: [0.25, 0.75] }),
       });
     });
 
@@ -815,7 +815,10 @@ describe('TestRunsService', () => {
     // decoded once at ingest, never again to group a screen at review time.
     it('groups from the signatures stored at ingest, without decoding anything', async () => {
       const signed = (id: string, signature: number[], overrides = {}) =>
-        sibling(id, { changeSignature: JSON.stringify(signature), ...overrides });
+        sibling(id, {
+          changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature }),
+          ...overrides,
+        });
       const testRun = signed('target', SAME_PALETTE, { customTags: '' });
       const matching = signed('locale-a', SAME_PALETTE);
       const different = signed('locale-c', OTHER_PALETTE);
@@ -835,10 +838,35 @@ describe('TestRunsService', () => {
       expect(result.skipped.map((item) => item.reason)).toEqual(['different change pattern']);
     });
 
+    // A signature only means anything under the threshold it was computed with.
+    // The in-memory memo has always keyed on that; the stored one has to too,
+    // or changing a project's comparison config silently mixes signatures from
+    // two different settings and grouping quietly gets worse.
+    it('recomputes when the project config has moved on since ingest', async () => {
+      const stale = JSON.stringify({ threshold: 0.9, includeAA: false, signature: SAME_PALETTE });
+      const testRun = sibling('target', { customTags: '', changeSignature: stale });
+      const matching = sibling('locale-a', { changeSignature: stale });
+
+      const built = await initMatchingService({
+        testRun,
+        siblings: [matching],
+        signatureOf: () => SAME_PALETTE,
+      });
+
+      const result = await built.service.getMatchingVariations(testRun.id);
+
+      const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString());
+      expect(asked.sort()).toEqual([testRun.imageName, matching.imageName].sort());
+      expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]);
+    });
+
     // builds ingested before the column existed, and runs compared by something
     // other than pixelmatch, still have to group
     it('falls back to computing for a run with nothing stored', async () => {
-      const testRun = sibling('target', { customTags: '', changeSignature: JSON.stringify(SAME_PALETTE) });
+      const testRun = sibling('target', {
+        customTags: '',
+        changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: SAME_PALETTE }),
+      });
       const unsigned = sibling('locale-a');
 
       const built = await initMatchingService({
diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts
index d97166a7..d1cb8c8a 100644
--- a/src/test-runs/test-runs.service.ts
+++ b/src/test-runs/test-runs.service.ts
@@ -4,7 +4,7 @@ import { IgnoreAreaDto } from './dto/ignore-area.dto';
 import { StaticService } from '../static/static.service';
 import { PrismaService } from '../prisma/prisma.service';
 import { Baseline, Prisma, TestRun, TestStatus, TestVariation } from '@prisma/client';
-import { DiffResult } from './diffResult';
+import { DiffResult, StampedSignature } from './diffResult';
 import { EventsGateway } from '../shared/events/events.gateway';
 import { TestRunResultDto } from '../test-runs/dto/testRunResult.dto';
 import { TestVariationsService } from '../test-variations/test-variations.service';
@@ -303,7 +303,7 @@ export class TestRunsService {
    * build does not pay for the same decodes twice.
    */
   private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise<number[] | null> {
-    const stored = parseStoredSignature(testRun.changeSignature, this.logger);
+    const stored = parseStoredSignature(testRun.changeSignature, config, this.logger);
     if (stored) {
       return stored;
     }
@@ -689,16 +689,30 @@ async function mapWithConcurrency<T, R>(items: T[], limit: number, task: (item:
 }
 
 /**
- * The signature stored on a run, or null when there is none to read. Bad JSON
- * is not worth failing a review over: the caller falls back to computing it.
+ * The signature stored on a run, if it is still usable. Null — meaning
+ * "recompute" — when there is none, when it cannot be read, or when the
+ * project's comparison settings have moved on since it was written.
+ *
+ * That last case is the point. Comparing a signature taken at one threshold
+ * against a sibling's taken at another quietly makes grouping worse, and the
+ * reviewer has no way to tell why. The in-memory memo has always keyed on the
+ * same settings; this is the stored equivalent.
  */
-function parseStoredSignature(stored: string | null | undefined, logger: Logger): number[] | null {
+function parseStoredSignature(
+  stored: string | null | undefined,
+  config: PixelmatchConfig,
+  logger: Logger
+): number[] | null {
   if (!stored) {
     return null;
   }
   try {
-    const parsed = JSON.parse(stored);
-    return Array.isArray(parsed) && parsed.length > 0 ? parsed : null;
+    const parsed: StampedSignature = JSON.parse(stored);
+    if (!Array.isArray(parsed?.signature) || parsed.signature.length === 0) {
+      return null;
+    }
+    const sameConfig = parsed.threshold === config.threshold && parsed.includeAA === config.ignoreAntialiasing;
+    return sameConfig ? parsed.signature : null;
   } catch (error) {
     logger.warn(`Ignoring unreadable stored change signature: ${error}`);
     return null;

From b9b7c423db7a38a1fbab545d99302974ae5c19b7 Mon Sep 17 00:00:00 2001
From: "mykola.gervasyuk" <gervasyuk@gmail.com>
Date: Mon, 7 Sep 2026 12:14:56 +0300
Subject: [PATCH 3/3] fix: only trust a stored signature that still has the
 right shape
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A signature is a fixed-length histogram, and parseStoredSignature accepted
any non-empty array as one. Comparing a stored vector of one length against
a current one of another walks off the end of the shorter and scores the
similarity over undefined entries — it returns a number, and the number
means nothing. Nothing throws; grouping just quietly gets worse.

That is the same failure the config stamp was added for, one level down: if
COLOR_BUCKETS_PER_CHANNEL is ever changed, every signature already in the
database becomes the wrong shape. The length and element types are now
checked against what this build produces, so a stored signature that no
longer fits is recomputed instead of half-used.

Also types saveDiffResult's parameter as nullable. It genuinely is — a
comparison that produced nothing clears the row back to "new", the body has
always handled that, and a test passes null — but the signature claimed
otherwise, which is the sort of small lie that invites a real one later.

Both found by Copilot on #376.
---
 src/compare/libs/pixelmatch/signature.core.ts |  6 ++++
 src/test-runs/test-runs.service.spec.ts       | 31 +++++++++++++++++--
 src/test-runs/test-runs.service.ts            | 16 ++++++++--
 3 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts
index a2701fdb..822208d2 100644
--- a/src/compare/libs/pixelmatch/signature.core.ts
+++ b/src/compare/libs/pixelmatch/signature.core.ts
@@ -30,6 +30,12 @@ export interface SignatureJobOutput {
 // COLOR_BUCKETS_PER_CHANNEL^3 histogram buckets.
 const COLOR_BUCKETS_PER_CHANNEL = 4;
 
+// How many buckets a signature has. Exported so a stored one can be checked
+// against the shape this build produces: comparing signatures of two different
+// lengths yields a similarity score computed over undefined entries — a number,
+// and a meaningless one.
+export const SIGNATURE_LENGTH = COLOR_BUCKETS_PER_CHANNEL ** 3;
+
 // Longest side (px) images are downscaled to before computing the color
 // signature — keeps the histogram representative while cutting pixelmatch cost.
 const SIGNATURE_MAX_DIMENSION = 500;
diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts
index 4ed710e3..10a4f62e 100644
--- a/src/test-runs/test-runs.service.spec.ts
+++ b/src/test-runs/test-runs.service.spec.ts
@@ -1,6 +1,7 @@
 import { mocked } from 'jest-mock';
 import { Test, TestingModule } from '@nestjs/testing';
 import { SIGNATURE_CONCURRENCY, TestRunsService } from './test-runs.service';
+import { SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core';
 import { PrismaService } from '../prisma/prisma.service';
 import { StaticService } from '../static/static.service';
 import { TestStatus, TestRun, TestVariation } from '@prisma/client';
@@ -747,8 +748,12 @@ describe('TestRunsService', () => {
   });
 
   describe('getMatchingVariations', () => {
-    const SAME_PALETTE = [1, 0];
-    const OTHER_PALETTE = [0, 1];
+    // Full-length histograms, because a stored one is only reused when its shape
+    // matches what this build produces. Orthogonal, so they never match.
+    const histogram = (hot: number): number[] =>
+      Array.from({ length: SIGNATURE_LENGTH }, (_unused, index) => (index === hot ? 1 : 0));
+    const SAME_PALETTE = histogram(0);
+    const OTHER_PALETTE = histogram(SIGNATURE_LENGTH - 1);
 
     // A sibling of the reviewed screen. imageName doubles as the run's identity
     // in the signature mocks below, which see buffers rather than runs.
@@ -860,6 +865,28 @@ describe('TestRunsService', () => {
       expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]);
     });
 
+    // A signature is a fixed-length histogram. One of the wrong length compared
+    // against one of the right length gives a similarity score computed over
+    // undefined entries — a number, silently meaningless. The same self-
+    // invalidating guard as the config stamp: if the shape moves on, recompute.
+    it.each([
+      ['the wrong length', [1, 0]],
+      ['values that are not numbers', new Array(SIGNATURE_LENGTH).fill('nope')],
+    ])('recomputes a stored signature with %s', async (_case, signature) => {
+      const stored = JSON.stringify({ threshold: 0.1, includeAA: true, signature });
+      const testRun = sibling('target', { customTags: '', changeSignature: stored });
+
+      const built = await initMatchingService({
+        testRun,
+        siblings: [],
+        signatureOf: () => SAME_PALETTE,
+      });
+
+      await built.service.getMatchingVariations(testRun.id);
+
+      expect(built.compareGetChangeSignatureMock).toHaveBeenCalled();
+    });
+
     // builds ingested before the column existed, and runs compared by something
     // other than pixelmatch, still have to group
     it('falls back to computing for a run with nothing stored', async () => {
diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts
index d1cb8c8a..423a6705 100644
--- a/src/test-runs/test-runs.service.ts
+++ b/src/test-runs/test-runs.service.ts
@@ -15,7 +15,7 @@ import { UpdateTestRunDto } from './dto/update-test.dto';
 import { parseConfig } from '../compare/utils';
 import { DEFAULT_CONFIG } from '../compare/libs/pixelmatch/pixelmatch.service';
 import { PixelmatchConfig } from '../compare/libs/pixelmatch/pixelmatch.types';
-import { signaturesMatch } from '../compare/libs/pixelmatch/signature.core';
+import { signaturesMatch, SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core';
 
 @Injectable()
 export class TestRunsService {
@@ -366,7 +366,10 @@ export class TestRunsService {
     return this.findOne(id);
   }
 
-  async saveDiffResult(id: string, diffResult: DiffResult): Promise<TestRun> {
+  // Nullable because it genuinely is: a comparison that produced nothing at all
+  // clears the row back to "new". The body has always handled that; the
+  // signature just did not say so.
+  async saveDiffResult(id: string, diffResult: DiffResult | null): Promise<TestRun> {
     return this.prismaService.testRun
       .update({
         where: { id },
@@ -708,7 +711,14 @@ function parseStoredSignature(
   }
   try {
     const parsed: StampedSignature = JSON.parse(stored);
-    if (!Array.isArray(parsed?.signature) || parsed.signature.length === 0) {
+    // Shape first: a histogram of the wrong length, or one carrying anything
+    // that is not a number, compares against a current signature to produce a
+    // score that looks fine and means nothing. Recomputing is always safe.
+    const wellFormed =
+      Array.isArray(parsed?.signature) &&
+      parsed.signature.length === SIGNATURE_LENGTH &&
+      parsed.signature.every((value) => typeof value === 'number' && Number.isFinite(value));
+    if (!wellFormed) {
       return null;
     }
     const sameConfig = parsed.threshold === config.threshold && parsed.includeAA === config.ignoreAntialiasing;