From e8c29eecb1f9350f066d8e19f64b0de9e28f38b1 Mon Sep 17 00:00:00 2001 From: thesiti92 Date: Sat, 10 Oct 2026 09:32:34 -0400 Subject: [PATCH] Digest the cleaned error message so one error about many files groups once --- .../scripts/smoke-error-telemetry.mjs | 16 +++++--------- packages/review/src/error-telemetry.test.ts | 12 ++++++++++ packages/review/src/error-telemetry.ts | 22 +++++++++++-------- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/apps/review-desktop/scripts/smoke-error-telemetry.mjs b/apps/review-desktop/scripts/smoke-error-telemetry.mjs index 5e9eecc2d..e74b59feb 100644 --- a/apps/review-desktop/scripts/smoke-error-telemetry.mjs +++ b/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 ''", }, { name: "an e-mail address is replaced", throws: "no account for someone@example.com", - match: { hashOf: "no account for someone@example.com" }, expected: "", }, { name: "a token is replaced", throws: "rejected credential ghp_012345678901234567890123456789012345", - match: { hashOf: "rejected credential ghp_012345678901234567890123456789012345" }, expected: "", }, { name: "a password is replaced", throws: "connect failed: password=hunter2", - match: { hashOf: "connect failed: password=hunter2" }, expected: "", }, { 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 () => diff --git a/packages/review/src/error-telemetry.test.ts b/packages/review/src/error-telemetry.test.ts index 7516e5155..ef1f13a95 100644 --- a/packages/review/src/error-telemetry.test.ts +++ b/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( diff --git a/packages/review/src/error-telemetry.ts b/packages/review/src/error-telemetry.ts index dc1e03761..b01c4de2c 100644 --- a/packages/review/src/error-telemetry.ts +++ b/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; } }