Digest the cleaned error message so one error about many files groups once

apps/review-desktop/scripts/smoke-error-telemetry.mjs

@@ -57,12 +57,11 @@ function leakTokens(home) {
 /**
  * Each case: what the app throws, and the exact message the report must carry.
  *
- * Reports are matched to cases by the digest of the message that was thrown,
+ * Reports are matched to cases by the digest of the cleaned message,
  * not by arrival order. Order matching looked simpler until a run lost one
  * report and every later case failed against its neighbour's result, turning
- * one fault into six. `match` names the digest source; the engine case cannot
- * use one because the wording belongs to the JavaScript engine, so it is found
- * by its error class instead.
+ * one fault into six. The engine case cannot use one because the wording
+ * belongs to the JavaScript engine, so it is found by its error class instead.
  */
 function cases(home) {
   return [
@@ -79,32 +78,27 @@ function cases(home) {
     {
       name: "a path under the real home directory is replaced whole",
       throws: `ENOENT: no such file or directory, open '${home}/work/acme-repo/plan.md'`,
-      match: { hashOf: `ENOENT: no such file or directory, open '${home}/work/acme-repo/plan.md'` },
       expected:
         "ENOENT: no such file or directory, open '<REDACTED: user-file-path>'",
     },
     {
       name: "an e-mail address is replaced",
       throws: "no account for someone@example.com",
-      match: { hashOf: "no account for someone@example.com" },
       expected: "<REDACTED: Email>",
     },
     {
       name: "a token is replaced",
       throws: "rejected credential ghp_012345678901234567890123456789012345",
-      match: { hashOf: "rejected credential ghp_012345678901234567890123456789012345" },
       expected: "<REDACTED: GitHub Token>",
     },
     {
       name: "a password is replaced",
       throws: "connect failed: password=hunter2",
-      match: { hashOf: "connect failed: password=hunter2" },
       expected: "<REDACTED: Generic Secret>",
     },
     {
       name: "a rejected promise is reported too",
       rejects: "the review request did not complete",
-      match: { hashOf: "the review request did not complete" },
       expected: "the review request did not complete",
     },
   ];
@@ -299,9 +293,9 @@ export async function smokeErrorTelemetry({
     await browser.close();
 
     const matchesProbe = (event, probe) =>
-      probe.match.errorName
+      probe.match
         ? event.properties?.error_name === probe.match.errorName
-        : event.properties?.message_hash === digestOf(probe.match.hashOf);
+        : event.properties?.message_hash === digestOf(probe.expected);
 
     // Warm-up reports can arrive after the first one has been observed.
     const readProbeReports = async () =>

packages/review/src/error-telemetry.test.ts

@@ -194,6 +194,18 @@ describe("deriveErrorTelemetryProperties", () => {
     );
   });
 
+  it("gives one digest to an error that differs only by file path", () => {
+    const digest = (path: string) =>
+      deriveErrorTelemetryProperties({
+        name: "Error",
+        message: `diffr did not supply a text result for ${path}.`,
+      }).message_hash;
+
+    expect(digest("/Users/alice/acme/a.ts")).toBe(
+      digest("/Users/alice/acme/b.ts"),
+    );
+  });
+
   it("keeps the class name only when it is identifier-like", () => {
     expect(deriveErrorTelemetryProperties(raw).error_name).toBe("TypeError");
     expect(

packages/review/src/error-telemetry.ts

@@ -95,17 +95,21 @@ export function deriveErrorTelemetryProperties(
     const message = jsonString(cause.message);
 
     if (message !== undefined && message.length > 0) {
+      const cleaned = cleanTelemetryText(message, NO_DELETED_DIRECTORIES);
+
       // The digest goes on every report, cleaned message or not. It is what
       // groups the reports whose message does not survive the checks below.
-      derived.message_hash = hashErrorMessage(message);
-
-      if (!SCHEMA_ERROR_NAMES.has(derived.error_name ?? "")) {
-        const cleaned = cleanTelemetryText(message, NO_DELETED_DIRECTORIES);
-
-        // Ask the allowlist's own check before sending. Failing here rather
-        // than there keeps a message that the cleaner could not finish out of
-        // the payload entirely, instead of relying on the later gate.
-        if (isReportableCleanedMessage(cleaned)) derived.message = cleaned;
+      // Cleaned, so an error that names a file has one digest for all files.
+      derived.message_hash = hashErrorMessage(cleaned);
+
+      // Ask the allowlist's own check before sending. Failing here rather
+      // than there keeps a message that the cleaner could not finish out of
+      // the payload entirely, instead of relying on the later gate.
+      if (
+        !SCHEMA_ERROR_NAMES.has(derived.error_name ?? "") &&
+        isReportableCleanedMessage(cleaned)
+      ) {
+        derived.message = cleaned;
       }
     }