diff --git a/runner/apps/authoring/src/sentry.ts b/runner/apps/authoring/src/sentry.ts index 6303aef68..15365f600 100644 --- a/runner/apps/authoring/src/sentry.ts +++ b/runner/apps/authoring/src/sentry.ts @@ -271,12 +271,13 @@ export function reportDemoEvent(payload: MonitorPayload, context: DemoEventConte return; } if (!demoRelayBudget.admit(clean.kind, message, clean.stack)) return; - // DEV-2854: a recognised Tier-2 compiler diagnostic collapses across TS codes into one - // flat, constant-titled bucket instead of the per-message fingerprint below. Never fed - // into `demoRelayBudget.admit` above — that stays keyed on the raw message, so 20 - // distinct diagnostics in one bad editing session still consume 20 of - // `MONITOR_EVENT_CEILING` rather than collapsing and losing their `extra` after the - // first. See `tier2Report.ts` for why. + // DEV-2854 / DEV-2876: a recognised Tier-2 compiler diagnostic, or a recognised Tier-2 + // build-failure envelope, collapses into its own flat, constant-titled bucket instead of + // the per-message fingerprint below. Never fed into `demoRelayBudget.admit` above — that + // stays keyed on the raw message, so 20 distinct diagnostics in one bad editing session + // still consume 20 of `MONITOR_EVENT_CEILING` rather than collapsing and losing their + // `extra` after the first. See `tier2Report.ts` for why, and for why the two shapes get + // two fingerprints rather than one. const tier2 = tier2StderrReport(clean.kind, message); const tags: Record = { surface: DEMO_SURFACE, diff --git a/runner/apps/authoring/src/tier2Report.ts b/runner/apps/authoring/src/tier2Report.ts index 53594aa4f..4fd38839b 100644 --- a/runner/apps/authoring/src/tier2Report.ts +++ b/runner/apps/authoring/src/tier2Report.ts @@ -1,11 +1,16 @@ /** - * What a Tier-2 compiler-diagnostic `stderr` line becomes in Sentry (DEV-2854, Sentry - * DEMOS-3K/3M/4F/3H and friends). + * What a Tier-2 `stderr` line becomes in Sentry, for the two recognised shapes: + * + * - A **compiler diagnostic** (DEV-2854, Sentry DEMOS-3K/3M/4F/3H and friends) — esbuild/tsc + * reporting a TS error against the visitor's own edited source. + * - A **build-failure envelope** (DEV-2876, Sentry DEMOS-5Q/53/4Y/4W/4V) — the container's + * own "the bundle did not come out the other end" line, wrapping a duration and a + * wall-clock timestamp rather than a code frame. * * Split out of `sentry.ts` for the same reason as `tier1Report.ts` and `reportingGate.ts`: * that file pulls `@sentry/react` and reads `import.meta.env`, so `node --test` cannot * import it and nothing in it can be pinned by a unit test. Keep this module import-free — - * the grouping rule is the whole of what it decides, and `pipeline/tier2-report.test.mjs` + * the grouping rules are the whole of what it decides, and `pipeline/tier2-report.test.mjs` * imports it as source. * * DEV-2854 was filed against the wrong site and asked for a change that was already made: @@ -21,28 +26,65 @@ * diagnostic code — DEMOS-3M holds six distinct quoted identifiers in one issue. Keying * a new fingerprint on the code would be a no-op on top of that. * - * What is actually broken, and what this module fixes: + * What DEV-2854 actually fixed, and what DEV-2876 adds on top: + * + * 1. **Title flap (compiler diagnostic).** A fingerprint coarser than the message, with the + * raw message still in the title, means the issue title names whichever sample arrived + * last (the same defect `tier1Report.ts` documents for `COMPILE_TITLE`). DEMOS-3K's + * title says `',' expected` while its newest event says `')' expected`. + * 2. **Cross-code spread (compiler diagnostic).** One bad Angular-editing session mints 20+ + * distinct TS codes, each its own single-event issue, because nothing groups across + * codes. + * 3. **Title flap (build envelope), DEV-2876.** `normalizeMonitorMessage`'s ISO rule already + * flattened the build-envelope fingerprint (commit `2464f3325`) — the collapse itself + * shipped. But nothing replaced the title, and the envelope embeds both a duration and a + * timestamp, so a flat fingerprint got a title that is a permanent snapshot of one + * moment: DEMOS-5Q is frozen at `Application bundle generation failed. [0.505 seconds] - + * 2026-09-02T07:40:26.664Z` no matter which sample arrives next. Same shape, same defect, + * on DEMOS-53/4Y/4W/4V. + * + * The fix for both shapes is a flat fingerprint, a constant title, and whatever varies + * per-event (`ts_code`; the raw envelope line) preserved as a facet in `extra`/`tags` rather + * than folded into either. That is the **invariant to hold, restated once for both branches: + * constant title iff flat fingerprint.** A title that varies with the code, or with the + * envelope's duration, on a fingerprint that does not, would flap exactly like the pre-fix + * behaviour did — which is why there is no `"Tier-2 compile failed (TS1005)"` and no + * `"Tier-2 build failed (0.505s)"` middle option here. + * + * The two shapes get **two fingerprints**, not one shared bucket: the envelope says *that* + * the build failed, a compiler diagnostic says *why*. A visitor who edits their way past a + * TS error and then hits an envelope failure (or the reverse) would otherwise land both + * causes and effects in one issue, which makes both harder to read — see the "collision" + * paragraph below for the case where a single line can carry both shapes. * - * 1. **Title flap.** A fingerprint coarser than the message, with the raw message still in - * the title, means the issue title names whichever sample arrived last (the same - * defect `tier1Report.ts` documents for `COMPILE_TITLE`). DEMOS-3K's title says - * `',' expected` while its newest event says `')' expected`. - * 2. **Cross-code spread.** One bad Angular-editing session mints 20+ distinct TS codes, - * each its own single-event issue, because nothing groups across codes. + * Checked **in order, TS diagnostic first**, and that order is load-bearing but not because + * one shape is "more specific" than the other — they match disjoint prefixes in the common + * case. It is load-bearing because it makes the build branch *strictly additive* over the + * population this function classified before it existed: every message this function + * returned `null` for pre-DEV-2876 either isn't `stderr`, or fails the TS-diagnostic test — + * and the build branch only ever fires after that same TS-diagnostic test has already failed. + * So no message this function used to classify (as a compile diagnostic, or as `null`) can be + * reclassified by adding the build branch; the only messages that change behaviour are ones + * that were previously `null` and match the new pattern. That is a structural guarantee about + * the code, not a claim that the two regexes' matches never overlap. * - * The fix is a flat fingerprint across every recognised diagnostic, a constant title, and - * the code preserved as a facet (`ts_code`) rather than folded into either. That is the - * **invariant to hold: constant title iff flat fingerprint** — a title that varies with the - * code on a fingerprint that does not would flap exactly like today's does, which is why - * there is no `"Tier-2 compile failed (TS1005)"` middle option here. + * They CAN overlap, and it is reachable, not theoretical: `kind: "stderr"` is one of the + * fixed `MONITOR_KINDS` (`packages/runtime/src/monitor.ts`), and nothing stops a forged + * `postMessage` from the preview delivering a single multi-line `stderr` payload that + * carries a TS diagnostic line and a build-envelope line together. When that happens, the + * compiler-diagnostic branch wins (checked first) — the TS code is the actionable half of + * that combined line, and it keeps `ts_code` as a facet, whereas the envelope branch has + * nothing more specific to offer once a diagnostic has already been found. This has a test + * (`ENVELOPE_PLUS_TS` below), not a comment dismissing it as impossible. * - * The recogniser below is an **allowlist on purpose**: it matches a TS diagnostic code in - * diagnostic position and nothing else. NG codes (`NG8001`, `NG8002` — plausibly our own - * Angular starter's `HotTableModule` wiring), `Failure reason:`, `::…::` install-failure - * markers, vite/vue internal errors, and `Could not resolve` are all untouched **by - * construction** — they keep reporting through the unchanged bare-message path in - * `sentry.ts`, so nothing that should stay loud goes quiet by falling through a denylist - * gap. `pipeline/tier2-report.test.mjs` pins every one of those as a guard against a future + * The recogniser is an **allowlist on purpose**, now with two entries: it matches a TS + * diagnostic code in diagnostic position, or the build-failure envelope's own opening + * sentence, and nothing else. NG codes (`NG8001`, `NG8002` — plausibly our own Angular + * starter's `HotTableModule` wiring), `Failure reason:`, `::…::` install-failure markers, + * vite/vue internal errors, and `Could not resolve` are all untouched **by construction** — + * they keep reporting through the unchanged bare-message path in `sentry.ts`, so nothing + * that should stay loud goes quiet by falling through a denylist gap. + * `pipeline/tier2-report.test.mjs` pins every one of those as a guard against a future * rewrite that swaps this allowlist for a denylist. */ @@ -51,6 +93,11 @@ * grouping or titling. */ const TIER2_COMPILE_TITLE = "Tier-2 compile failed"; +/** The constant title for every recognised Tier-2 build-failure envelope, for all time. The + * raw line — duration and ISO timestamp included — rides in `extra.buildFailure` instead, + * which takes no part in grouping or titling. */ +const TIER2_BUILD_TITLE = "Tier-2 build failed"; + /** A TS diagnostic code in diagnostic position — the code immediately followed by a colon * and a space, as esbuild/tsc emit it (`TS1005: ',' expected.`). Requires a word boundary * before `TS` and the trailing `: ` so a bare `TS1005` mentioned in prose, with no code @@ -63,6 +110,17 @@ const TS_CODE_IN_DIAGNOSTIC_POSITION = /\bTS\d{4,5}:\s/; * must agree on what counts as "a code", so change them together. */ const TS_CODE_GLOBAL = /\bTS\d{4,5}(?=:\s)/g; +/** The container's build-failure envelope, anchored at line start. `reportDemoEvent`'s + * caller (`container.ts`'s stderr relay) trims each line before it ever reaches here, so an + * anchor at `^` is safe and deliberately excludes a mid-line prose mention of the same + * sentence (`BUNDLE_IN_PROSE` below). Does NOT require the trailing `[N seconds] - + * ` suffix — only the opening sentence — so a toolchain version that drops + * the timing still collapses into the same bucket. The literal `\.` (not `.`) means + * `Application bundle generation complete.` does not match: the two sentences differ only + * in that one word, and matching the wildcard would fold a success line into a failure + * bucket. */ +const BUNDLE_GENERATION_FAILED = /^Application bundle generation failed\./; + export interface Tier2StderrReport { fingerprint: string[]; tags: Record; @@ -73,36 +131,65 @@ export interface Tier2StderrReport { /** * Decide how a Tier-2 `stderr` line is reported, or that it is not. * - * `null` for anything other than `kind === "stderr"` with a recognised TS diagnostic code: - * the caller keeps today's per-message fingerprint and title unchanged. Not reclassified, - * not merged, no synthetic title — an unrecognised line needs nothing extra to keep working, - * since `normalizeMonitorMessage`'s quoted-string rule already groups the no-code case - * (DEMOS-3H's `Unexpected "}"` / `Unexpected ","`). + * `null` for anything other than `kind === "stderr"` with a recognised TS diagnostic code or + * a recognised build-failure envelope: the caller keeps today's per-message fingerprint and + * title unchanged. Not reclassified, not merged, no synthetic title — an unrecognised line + * needs nothing extra to keep working, since `normalizeMonitorMessage`'s rules already group + * the no-code case (DEMOS-3H's `Unexpected "}"` / `Unexpected ","`) and the envelope's own + * ISO timestamp (DEV-2876's flat-fingerprint half, already shipped). */ export function tier2StderrReport(kind: string, message: string): Tier2StderrReport | null { if (kind !== "stderr") return null; - if (!TS_CODE_IN_DIAGNOSTIC_POSITION.test(message)) return null; - const codes = new Set(); - for (const match of message.matchAll(TS_CODE_GLOBAL)) codes.add(match[0]); - // First match wins for the tag; a line naming more than one distinct code (a repeated, - // truncated diagnostic block) omits the tag rather than pick arbitrarily. The - // fingerprint below is unaffected either way — it never carries a code. - const singleCode = codes.size === 1 ? [...codes][0] : undefined; + if (TS_CODE_IN_DIAGNOSTIC_POSITION.test(message)) { + const codes = new Set(); + for (const match of message.matchAll(TS_CODE_GLOBAL)) codes.add(match[0]); + // First match wins for the tag; a line naming more than one distinct code (a repeated, + // truncated diagnostic block) omits the tag rather than pick arbitrarily. The + // fingerprint below is unaffected either way — it never carries a code. + const singleCode = codes.size === 1 ? [...codes][0] : undefined; + + return { + // Flat, never keyed on the code: per-code keying leaves ~20 issues per bad editing + // session and is unbounded in the TS vocabulary. `framework` / `tier` deliberately + // stay out too — they are already tags in `sentry.ts`, and the house rule there is + // that instrumentation facets go beside the fingerprint, never inside it. + fingerprint: ["demo-runtime", "stderr", "tier2-compile"], + tags: { + kind_class: "tier2-compile", + ...(singleCode ? { ts_code: singleCode } : {}), + }, + // The raw line, verbatim and bounded/host-redacted upstream by `sanitizeMonitorPayload` + // — never in `display` or the fingerprint, which is what keeps the title constant. + extra: { compileDiagnostic: message }, + display: TIER2_COMPILE_TITLE, + }; + } + + if (BUNDLE_GENERATION_FAILED.test(message)) { + return { + // Distinct third element from `tier2-compile`'s: the envelope says *that* the build + // failed, a diagnostic says *why*. Keeping them separate means a visitor's TS typo and + // a container-side build failure never get merged into one issue that mixes cause and + // effect. + fingerprint: ["demo-runtime", "stderr", "tier2-build"], + tags: { kind_class: "tier2-build" }, + // The raw line — duration and ISO timestamp included — verbatim and bounded/ + // host-redacted upstream by `sanitizeMonitorPayload`. A distinct key from + // `compileDiagnostic` above so `extra` stays self-describing per bucket. Nothing here + // is faceted into a tag: the wall time measures how far esbuild got before erroring (a + // function of project size and container load, not of the defect), so a bucketed + // `"<1s"`/`"1-5s"` tag would be a metric wearing a tag's clothes with no named + // consumer — a tag has to earn its cardinality the way `ts_code` does above by + // slicing the bucket along a fault-class axis, and duration doesn't. The ISO + // timestamp is the *container's* clock at build time, not a duplicate of Sentry's own + // event timestamp, which is why it is kept as recoverable context in `extra` rather + // than dropped — but it is still not a fault-class discriminator, so it stays out of + // the title and the fingerprint exactly like the duration. + extra: { buildFailure: message }, + display: TIER2_BUILD_TITLE, + }; + } - return { - // Flat, never keyed on the code: per-code keying leaves ~20 issues per bad editing - // session and is unbounded in the TS vocabulary. `framework` / `tier` deliberately - // stay out too — they are already tags in `sentry.ts`, and the house rule there is - // that instrumentation facets go beside the fingerprint, never inside it. - fingerprint: ["demo-runtime", "stderr", "tier2-compile"], - tags: { - kind_class: "tier2-compile", - ...(singleCode ? { ts_code: singleCode } : {}), - }, - // The raw line, verbatim and bounded/host-redacted upstream by `sanitizeMonitorPayload` - // — never in `display` or the fingerprint, which is what keeps the title constant. - extra: { compileDiagnostic: message }, - display: TIER2_COMPILE_TITLE, - }; + return null; } diff --git a/runner/pipeline/tier2-report.test.mjs b/runner/pipeline/tier2-report.test.mjs index a1ba40265..d199c8c2a 100644 --- a/runner/pipeline/tier2-report.test.mjs +++ b/runner/pipeline/tier2-report.test.mjs @@ -28,6 +28,35 @@ const TS2345_REPEATED = // A line naming two genuinely distinct codes. const TS_MULTI_CODE = `${TS1005} ${TS2304}`; +// DEV-2876 / Sentry DEMOS-5Q, DEMOS-53, DEMOS-4Y, DEMOS-4W, DEMOS-4V. The build-failure +// envelope's fingerprint was already flattened by `normalizeMonitorMessage`'s ISO rule +// (commit `2464f3325`), but the raw line — duration and timestamp included — was still the +// title, so each bucket was permanently named after whichever sample arrived last. DEMOS-5Q +// is frozen at the exact `BUNDLE_5Q` string below. + +const BUNDLE_5Q = `Application bundle generation failed. [0.505 seconds] - 2026-09-02T07:40:26.664Z`; +const BUNDLE_4V = `Application bundle generation failed. [1.595 seconds] - 2026-08-27T14:20:04.952Z`; +// No timing suffix at all — a toolchain version that drops it must still collapse. +const BUNDLE_NO_TIMING = `Application bundle generation failed.`; +// The success sibling: differs from the failure sentence in one word, so the anchor's +// literal `\.` (not `.`) is what keeps this from matching. +const BUNDLE_COMPLETE = `Application bundle generation complete. [0.412 seconds] - 2026-09-02T07:41:01.112Z`; +// A mid-line mention, not a line that opens with the sentence — the `^` anchor is what +// excludes this, and `container.ts`'s `line = raw.trim()` is what makes anchoring safe. +const BUNDLE_IN_PROSE = `esbuild said Application bundle generation failed. earlier`; +// Real boot-script narration from `container.ts` (`::frozen install failed for custom +// metadata; retrying non-frozen::`) — one of the `::…::` install-failure markers the +// existing allowlist already excludes by construction; re-asserted here against both +// recognisers, not just the compile one. +const FROZEN_RETRY = `::frozen install failed for custom metadata; retrying non-frozen::`; +// Real vite dev-server output (see `pipeline/monitor-stderr-relay.test.mjs`) — internal to +// vite, not a TS diagnostic and not a build envelope. +const VITE_INTERNAL = `[vite] Internal server error: hot is not defined`; +// A single stderr line carrying both recognised shapes — reachable via a forged +// `postMessage`, not merely theoretical (`kind: "stderr"` is one of the fixed +// `MONITOR_KINDS`). Tie-break goes to the compile branch: it is checked first. +const ENVELOPE_PLUS_TS = `${BUNDLE_5Q} ${TS1005}`; + // --- Load-bearing: demonstrably false on master, true after --------------------------- test("TS1005 and TS2304 share one fingerprint (cross-code-spread defect)", () => { @@ -113,3 +142,84 @@ test("guard: kind !== 'stderr' returns null even with a recognised code", () => assert.equal(tier2StderrReport("console-error", TS1005), null); assert.equal(tier2StderrReport("error", TS1005), null); }); + +// --- DEV-2876: build-failure envelope ---------------------------------------------------- +// --- Load-bearing: demonstrably false on master, true after --------------------------- + +test("DEMOS-5Q and DEMOS-4V share one fingerprint, equal to the tier2-build fingerprint", () => { + // The flat grouping itself already shipped in `2464f3325` via `normalizeMonitorMessage`'s + // ISO rule — this pins that the explicit fingerprint below does not re-shard what that + // rule already collapsed. + const a = tier2StderrReport("stderr", BUNDLE_5Q); + const b = tier2StderrReport("stderr", BUNDLE_4V); + assert.deepEqual(a.fingerprint, b.fingerprint); + assert.deepEqual(a.fingerprint, ["demo-runtime", "stderr", "tier2-build"]); +}); + +test("the display string is the same constant across BUNDLE_5Q / BUNDLE_4V / BUNDLE_NO_TIMING, and is not the raw message", () => { + const displays = [BUNDLE_5Q, BUNDLE_4V, BUNDLE_NO_TIMING].map( + (message) => tier2StderrReport("stderr", message).display, + ); + assert.deepEqual(new Set(displays), new Set(["Tier-2 build failed"])); + // On master this equals the raw message (in fact tier2StderrReport returns null and + // `display` doesn't exist at all), so it is not constant across samples today. + assert.notEqual(tier2StderrReport("stderr", BUNDLE_5Q).display, BUNDLE_5Q); +}); + +test("the envelope fingerprint and display differ from the compile branch's", () => { + const build = tier2StderrReport("stderr", BUNDLE_5Q); + const compile = tier2StderrReport("stderr", TS1005); + assert.notDeepEqual(build.fingerprint, compile.fingerprint); + assert.notEqual(build.display, compile.display); +}); + +test("the raw envelope line reaches extra.buildFailure verbatim, and nowhere else", () => { + const r = tier2StderrReport("stderr", BUNDLE_5Q); + assert.equal(r.extra.buildFailure, BUNDLE_5Q); + assert.equal(r.display, "Tier-2 build failed"); + assert.ok(!r.display.includes("0.505")); + assert.ok(!r.display.includes("2026-09-02")); + assert.ok(!r.fingerprint.join("|").includes("0.505")); + assert.ok(!r.fingerprint.join("|").includes("2026-09-02")); + // Pins the no-duration-tag decision so a later "helpful" addition fails here. + assert.equal(r.tags.build_duration, undefined); +}); + +test("the envelope is tagged tier2-build and carries no ts_code", () => { + const r = tier2StderrReport("stderr", BUNDLE_5Q); + assert.equal(r.tags.kind_class, "tier2-build"); + assert.equal(r.tags.ts_code, undefined); +}); + +// --- Guards: pass either way today, exist to fail on a future denylist / anchor slip ---- + +test("guard: 'Application bundle generation complete.' and a mid-line mention both return null", () => { + assert.equal(tier2StderrReport("stderr", BUNDLE_COMPLETE), null); + assert.equal(tier2StderrReport("stderr", BUNDLE_IN_PROSE), null); +}); + +test("guard: FROZEN_RETRY, VITE_INTERNAL, RESOLVE, NG8001, NG8002, and 'Failure reason:' all return null", () => { + // Deliberately overlaps the existing guards above (`:98-106`-ish): those were written + // against one recogniser, this re-asserts them against two. + assert.equal(tier2StderrReport("stderr", FROZEN_RETRY), null); + assert.equal(tier2StderrReport("stderr", VITE_INTERNAL), null); + assert.equal(tier2StderrReport("stderr", RESOLVE), null); + assert.equal(tier2StderrReport("stderr", NG8001), null); + assert.equal(tier2StderrReport("stderr", NG8002), null); + assert.equal(tier2StderrReport("stderr", FAILURE), null); +}); + +test("guard: kind other than 'stderr' returns null even with a recognised envelope", () => { + assert.equal(tier2StderrReport("console-error", BUNDLE_5Q), null); + assert.equal(tier2StderrReport("error", BUNDLE_5Q), null); +}); + +// --- Order guard: TS-first is load-bearing, not a specificity claim --------------------- + +test("order guard: a line carrying both shapes resolves as the compile branch, not the build branch", () => { + // Passes on revert too (still matches the TS regex on its own) — it exists to fail if + // someone reorders the branches so the build check runs first. + const r = tier2StderrReport("stderr", ENVELOPE_PLUS_TS); + assert.deepEqual(r.fingerprint, ["demo-runtime", "stderr", "tier2-compile"]); + assert.equal(r.tags.ts_code, "TS1005"); +});