fix(runner): inject the Tier-1 reporter on one line (DEV-2557)

runner/packages/runtime/src/monitor.ts

@@ -581,6 +581,61 @@ export const REPORTER_SOURCE = `(function () {
 })();
 `;
 
+/**
+ * The reporter as a *single physical line*, for prepending to a JS module entry.
+ *
+ * DEV-2557. Whatever we prepend to the entry shifts every position the bundler
+ * reports for that file, and the visitor is shown those positions verbatim. Inlining
+ * the reporter body cost 226 lines at the releases the Sentry events were tagged
+ * with, and 283 after DEV-2552 grew it — which is how a syntax error in a 70-line
+ * file came back as "(257:22)". One line of prefix means one line of shift.
+ *
+ * Why an indirect eval and not a separate module the entry imports: a new module
+ * would put a specifier the author never wrote into the graph, next to a moving entry
+ * path, entangled with `resolveSandboxEntry`, `sameFiles` and `stampEntry`, and would
+ * depend on the classic bundler evaluating an injected dependency before the entry
+ * body. Its failure mode is a blank preview. This form has zero graph interaction.
+ *
+ * Why `(0,eval)` and not `eval`: the indirect form evaluates in global scope, where
+ * the reporter's bare `window`/`parent`/`document`/`location`/`XMLHttpRequest`
+ * resolve, and where it leaks no bindings into the bundler's module wrapper.
+ *
+ * Why the try/catch: if `eval` is ever unavailable, that must cost monitoring on this
+ * path and never the demo. An entry that resolves to an HTML file (`parcel`/`static`
+ * with an `htmlEntry` — see `resolveSandboxEntry`) keeps the `<script>` injection as a
+ * working channel; anything else, the vue entry included, reaches the reporter only
+ * through this one. Verified live rather than inferred, since the catch would hide the
+ * failure: `window.__hotRunnerMonitor` is true inside the real Sandpack preview iframe
+ * for both a vue and a react entry.
+ *
+ * Still byte-deterministic: a pure function of a constant, computed once at module
+ * load — nothing hashed, padded, timestamped or randomised — so `sameFiles` keeps
+ * skipping the no-op compile (see `injectReporter` below).
+ *
+ * `alreadyInjected` still matches it: `JSON.stringify` escapes the quotes around
+ * MONITOR_MESSAGE_TYPE but leaves the string itself verbatim, so a double injection
+ * stays a no-op.
+ *
+ * One physical line is not free, and the cost lands somewhere non-obvious: babel's code
+ * frame prints the two lines above the fault verbatim, so a syntax error on authored
+ * line 1 or 2 renders all 12.6 KB of this into the compile message ahead of the line
+ * that is actually wrong, and `MONITOR_COMPILE_MESSAGE_MAX` then cuts the diagnostic off
+ * (measured: 289 characters of usable message with the reporter inlined, 12,872 with it
+ * on one line). `boundCompileMessage` in sandpack.ts therefore replaces this exact
+ * constant with a marker before the cap runs — `stripInjectedReporter`, which is
+ * coupled to this constant on purpose. Do not change the shape of this line without
+ * checking that strip still fires.
+ *
+ * What this does NOT fix: the entry is generally transpiled before the injection —
+ * `transpileFilesForParcel` for the parcel entries, and babel does not use
+ * `retainLines` — so a reported line is still a *compiled* line. Do not read this as
+ * "line numbers are now correct" for any entry: measured on the vue starter, a syntax
+ * error typed on authored line 11 reports line 14, the residual +3 coming from that
+ * entry's own TS transform. This removes the distortion we add (the same error
+ * reported line 316 before), and only source maps can close the rest.
+ */
+export const REPORTER_MODULE_LINE = `try{(0,eval)(${JSON.stringify(REPORTER_SOURCE)})}catch(e){}`;
+
 /** True when `source` already carries the reporter. */
 function alreadyInjected(source: string): boolean {
   return source.indexOf(MONITOR_MESSAGE_TYPE) !== -1;
@@ -616,6 +671,12 @@ export function injectReporterIntoHtml(html: string): string {
  * `static` environments (which is every Tier-1 example that has one), a JS module
  * otherwise. Both are handled, because a module entry still runs before the demo.
  *
+ * The module branch prepends `REPORTER_MODULE_LINE`, which is one physical line, so
+ * the compile positions the visitor is shown are off by one rather than by the
+ * reporter's length (DEV-2557). The HTML branch is deliberately left as it is: it is
+ * Tier-2's only monitoring channel (`workers/api/src/monitor-inject.ts`) and widening
+ * the eval bet to it wants its own decision.
+ *
  * Byte-deterministic by construction: no timestamp, no id, no ordering that
  * depends on iteration. `SandpackRuntime.sameFiles` skips the compile when the
  * sandbox is unchanged, and a reporter that differed between two builds of the
@@ -631,6 +692,6 @@ export function injectReporter(files: Record<string, string>, entryPath: string)
   if (alreadyInjected(source)) return files;
   const injected = entryPath.toLowerCase().endsWith(".html")
     ? injectReporterIntoHtml(source)
-    : REPORTER_SOURCE + "\n" + source;
+    : REPORTER_MODULE_LINE + "\n" + source;
   return { ...files, [entryPath]: injected };
 }

runner/packages/runtime/src/sandpack.ts

@@ -22,6 +22,7 @@ import { applyDepShims } from "./dep-shims.js";
 import { resolveSandboxEntry, toParcelEntry } from "./sandbox-entry.js";
 import {
   MONITOR_COMPILE_MESSAGE_MAX,
+  REPORTER_MODULE_LINE,
   injectReporter,
   redactPreviewHosts,
   truncateMessage,
@@ -186,6 +187,37 @@ const COMPILE_ERROR_FALLBACK = "Sandpack compile error";
  *  that says what is wrong; the marker is kept so the line still reads. */
 const INLINE_SOURCE_MAP = /sourceMappingURL=data:[^\s'"*]*/gi;
 
+/**
+ * What the injected reporter line is replaced with inside a compile message
+ * (DEV-2557). Kept, rather than deleted, so the code frame's line numbering still
+ * lines up with what the message says.
+ */
+const INJECTED_REPORTER_MARKER = "<hot-runner monitor>";
+
+/**
+ * Strip the monitor's own injected line out of a compile message (DEV-2557).
+ *
+ * `REPORTER_MODULE_LINE` is one 12.6 KB physical line at the top of the module entry,
+ * and babel's code frame prints the two lines *above* the fault verbatim — so a syntax
+ * error on authored line 1 or 2 renders that whole blob into the message before the
+ * offending line is reached. Measured on the vue starter's entry with an unterminated
+ * string on line 1: 289 characters of usable message with a caret when the reporter was
+ * still inlined, 12,872 with it on one line, and `MONITOR_COMPILE_MESSAGE_MAX` then cuts
+ * at 2,000 — so the diagnostic line and its caret were gone. That is DEV-2550's
+ * buried-diagnostic failure (DEMOS-15) coming back on the same channel, from our own
+ * bytes rather than from a source map.
+ *
+ * Matched against the exported constant rather than a pattern, deliberately: the strip
+ * has to stay coupled to the injection. A regex over the injected *shape* would keep
+ * passing its own tests while silently ceasing to match a reworded injection, and the
+ * burying would return with nothing red. `pipeline/sandpack-reload.test.mjs` runs a real
+ * babel over a real `injectReporter` output and asserts the visitor's own source
+ * survives the cap, so a change to either side fails there rather than in production.
+ */
+function stripInjectedReporter(message: string): string {
+  return message.split(REPORTER_MODULE_LINE).join(INJECTED_REPORTER_MARKER);
+}
+
 /**
  * Bound the bundler's `show-error` string (DEV-2550).
  *
@@ -197,11 +229,12 @@ const INLINE_SOURCE_MAP = /sourceMappingURL=data:[^\s'"*]*/gi;
  * `sanitizeMonitorPayload`, container stderr through `truncateMessage`; this is the
  * same treatment for this one.
  *
- * Order is load-bearing twice over. Source maps are stripped *first*, so the cap
- * spends its budget on the diagnostic instead of on half a blob. Hosts are then
- * redacted *before* truncation — the security property monitor.ts documents on
- * `bound()`: truncating first can cut a preview hostname in half and strand a live
- * session token in a form the redactor no longer recognises.
+ * Order is load-bearing three times over. Source maps and the injected reporter line
+ * (see `stripInjectedReporter`) are stripped *first*, so the cap spends its budget on
+ * the diagnostic instead of on half a blob. Hosts are then redacted *before* truncation
+ * — the security property monitor.ts documents on `bound()`: truncating first can cut a
+ * preview hostname in half and strand a live session token in a form the redactor no
+ * longer recognises.
  *
  * Takes `unknown` and coerces, matching `truncateMessage`: the payload crossed an
  * origin boundary from a page running the visitor's code, so its shape is not a
@@ -233,7 +266,8 @@ function boundCompileMessage(value: unknown): string {
   }
   if (raw.trim() === "") return COMPILE_ERROR_FALLBACK;
   const withoutMaps = raw.replace(INLINE_SOURCE_MAP, "sourceMappingURL=<omitted>");
-  return truncateMessage(redactPreviewHosts(withoutMaps), MONITOR_COMPILE_MESSAGE_MAX);
+  const withoutReporter = stripInjectedReporter(withoutMaps);
+  return truncateMessage(redactPreviewHosts(withoutReporter), MONITOR_COMPILE_MESSAGE_MAX);
 }
 
 export class SandpackRuntime implements DemoRuntime {

runner/pipeline/monitor-inject.test.mjs

@@ -1,5 +1,6 @@
 import test from "node:test";
 import assert from "node:assert/strict";
+import vm from "node:vm";
 import * as acorn from "acorn";
 import {
   MONITOR_BREADCRUMB_CEILING,
@@ -9,6 +10,7 @@ import {
   MONITOR_MESSAGE_TYPE,
   MONITOR_STACK_MAX,
   MONITOR_URL_MAX,
+  REPORTER_MODULE_LINE,
   REPORTER_SOURCE,
   createMonitorBudget,
   injectReporter,
@@ -70,6 +72,61 @@ test("prepends to a JS module entry", () => {
   assert.ok(out["/src/main.js"].trimEnd().endsWith("grid();"), "demo source stays last");
 });
 
+// ---- DEV-2557: the module entry may shift the demo by at most one line -------
+//
+// Every position the bundler reports for the entry file is offset by whatever we
+// prepended to it. Inlining the reporter body cost 226 lines at the releases the
+// Sentry events were tagged with, and 283 after DEV-2552 grew it — which is how a
+// syntax error in a 70-line file came back as "(257:22)". The injection is now one
+// physical line, so the distortion *we* add is one line, and it stays that way only
+// because these tests check it.
+
+const MODULE_ENTRY = "/src/main.js";
+const MODULE_SOURCE = ["import { grid } from './grid';", "", "// a comment", "grid();", ""].join("\n");
+
+test("DEV-2557: a module entry is shifted by exactly one line", () => {
+  // Computed from the split rather than asserted against a constant: a constant is
+  // what put the wrong number in the ticket in the first place.
+  const injected = injectReporter({ [MODULE_ENTRY]: MODULE_SOURCE }, MODULE_ENTRY)[MODULE_ENTRY];
+  const before = MODULE_SOURCE.split("\n");
+  const after = injected.split("\n");
+
+  assert.equal(after.length - before.length, 1, "exactly one line added");
+  for (let i = 0; i < before.length; i += 1) {
+    assert.equal(after[i + 1], before[i], `authored line ${i + 1} lands on reported line ${i + 2}`);
+  }
+  assert.ok(after[0].includes(MONITOR_MESSAGE_TYPE), "and that one line is the reporter");
+});
+
+test("DEV-2557: the injected module prefix carries no line terminator", () => {
+  // `JSON.stringify` escapes \n and \r but NOT U+2028/U+2029, which JS treats as
+  // line terminators. REPORTER_SOURCE is ASCII today; this is the only thing
+  // standing between a future non-ASCII edit and a silent return of the offset.
+  // Built with `fromCharCode` rather than written literally, so no editor or
+  // formatter can quietly normalise away the characters this test exists to reject.
+  for (const [name, ch] of [
+    ["LF", "\n"],
+    ["CR", "\r"],
+    ["U+2028", String.fromCharCode(0x2028)],
+    ["U+2029", String.fromCharCode(0x2029)],
+  ]) {
+    assert.equal(REPORTER_MODULE_LINE.includes(ch), false, `prefix must not contain ${name}`);
+  }
+});
+
+test("DEV-2557: module-entry injection is idempotent and byte-deterministic", () => {
+  // The existing pair of tests covers the HTML entry only. `alreadyInjected` keys on
+  // MONITOR_MESSAGE_TYPE appearing in the source, and the marker has to survive the
+  // JSON escaping of the reporter body for a double injection to stay a no-op.
+  const files = { [MODULE_ENTRY]: MODULE_SOURCE };
+  const a = injectReporter(files, MODULE_ENTRY);
+  const b = injectReporter(files, MODULE_ENTRY);
+  assert.equal(a[MODULE_ENTRY], b[MODULE_ENTRY], "two builds of the same source are byte-identical");
+
+  const twice = injectReporter(a, MODULE_ENTRY);
+  assert.equal(twice, a, "second injection returns the same object, untouched");
+});
+
 test("injection is byte-deterministic", () => {
   // `sameFiles` skips the compile when the sandbox is unchanged. A reporter that
   // varied between builds would make every keystroke a real diff and defeat it.
@@ -103,16 +160,17 @@ test("falls back to body, then to a prepend, when there is no head", () => {
 
 // ---- the reporter, executed -------------------------------------------------
 
-/** Run REPORTER_SOURCE against stubs and return the harness. Bare `window`,
- *  `parent`, `console`, `document` and `XMLHttpRequest` in the reporter resolve to
- *  these parameters, so no DOM implementation is needed.
+/** The stub environment the reporter is executed against — no DOM implementation
+ *  needed. Extracted from `runReporter` so the DEV-2557 vm test can install the
+ *  *same* stubs as real context globals and compare the two runs; the eval form
+ *  evaluates in global scope and can never see `new Function` parameters.
  *
  *  Options, each present because one behaviour can only be reached through it:
  *  `fetch` and `XMLHttpRequest` install the transports the reporter wraps,
  *  `location` overrides the page's own host (pass `undefined` to make it
  *  unreadable), and `brokenAnchor` makes `document.createElement` throw, which is
  *  the only way to produce a network event with no attributable URL. */
-function runReporter(options = {}) {
+function makeReporterStubs(options = {}) {
   const sent = [];
   const listeners = new Map();
   const passthrough = [];
@@ -153,27 +211,38 @@ function runReporter(options = {}) {
   // from — the reporter must strip it from everything it sends.
   const location = "location" in options ? options.location : { host: PREVIEW_HOST };
 
-  // eslint-disable-next-line no-new-func
-  new Function("window", "parent", "console", "document", "XMLHttpRequest", "location", REPORTER_SOURCE)(
-    win,
-    parent,
-    consoleStub,
-    document,
-    options.XMLHttpRequest,
-    location,
-  );
-
   return {
     sent,
     passthrough,
     win,
+    parent,
+    document,
+    location,
+    XMLHttpRequest: options.XMLHttpRequest,
     console: consoleStub,
     fire(type, event) {
       for (const cb of listeners.get(type) ?? []) cb(event);
     },
   };
 }
 
+/** Run REPORTER_SOURCE against those stubs and return the harness. Bare `window`,
+ *  `parent`, `console`, `document` and `XMLHttpRequest` in the reporter resolve to
+ *  these parameters. */
+function runReporter(options = {}) {
+  const h = makeReporterStubs(options);
+  // eslint-disable-next-line no-new-func
+  new Function("window", "parent", "console", "document", "XMLHttpRequest", "location", REPORTER_SOURCE)(
+    h.win,
+    h.parent,
+    h.console,
+    h.document,
+    h.XMLHttpRequest,
+    h.location,
+  );
+  return h;
+}
+
 /** A fresh XHR stub class per call — the reporter patches `prototype.open`, so a
  *  shared class would end up wrapped once per harness. */
 function makeFakeXHR() {
@@ -207,6 +276,47 @@ test("reporter relays an uncaught error", () => {
   assert.ok(isMonitorPayload(h.sent[0]), "payload passes the parent's own validation");
 });
 
+test("DEV-2557: the one-line eval form installs the same hooks as the inlined reporter", () => {
+  // The one-line injection buys its line count with an indirect eval, so the eval
+  // path has to be *executed*, not assumed — the DEV-2129 lesson again. `runReporter`
+  // above cannot do it: it hands the stubs in as `new Function` parameters, and an
+  // indirect eval evaluates in global scope where those bindings do not exist. Hence
+  // a vm context, where the same stubs are real globals.
+  //
+  // Asserted as an equivalence against the inlined run rather than as "something was
+  // sent": the payload is what the parent validates and Sentry receives, and a
+  // divergence in it is the failure that would matter.
+  const inlined = runReporter();
+  inlined.fire("error", { error: new Error("boom"), message: "boom" });
+
+  const evaled = makeReporterStubs();
+  const context = vm.createContext({
+    window: evaled.win,
+    parent: evaled.parent,
+    console: evaled.console,
+    document: evaled.document,
+    location: evaled.location,
+    URL,
+  });
+  const injected = injectReporter({ [MODULE_ENTRY]: "globalThis.__demoRan = true;" }, MODULE_ENTRY)[MODULE_ENTRY];
+  vm.runInContext(injected, context, { filename: MODULE_ENTRY });
+
+  assert.equal(context.__demoRan, true, "the demo's own source still evaluates after the prefix");
+
+  evaled.fire("error", { error: new Error("boom"), message: "boom" });
+  assert.equal(evaled.sent.length, 1, "the eval-installed listener fired");
+  assert.ok(isMonitorPayload(evaled.sent[0]), "payload passes the parent's own validation");
+  // Structural compare: the payload is built inside the vm realm, so its prototype is
+  // not this realm's Object.prototype and a strict deep-equal would fail on that
+  // alone. The stack differs only by the two `new Error` call sites.
+  assert.deepEqual(
+    { ...evaled.sent[0], stack: undefined },
+    { ...inlined.sent[0], stack: undefined },
+    "eval-injected reporter produces the same payload as the inlined one",
+  );
+  assert.ok(evaled.sent[0].stack.includes("Error: boom"), "stack still relayed");
+});
+
 test("reporter relays an unhandled rejection", () => {
   const h = runReporter();
   h.fire("unhandledrejection", { reason: new Error("nope") });
@@ -829,6 +939,18 @@ test("REPORTER_SOURCE parses as ES5", () => {
   // prepended to, where a parse failure is a blank Tier-1 preview.
   //
   // Parsed, not grepped: a syntax allowlist is a list of the mistakes already made.
+  //
+  // This must stay pointed at REPORTER_SOURCE and never at the injected output.
+  // Since DEV-2557 the module entry carries the reporter as a single JSON string
+  // literal, which parses at ES5 no matter what is inside it — repointing the guard
+  // there would check nothing, forever.
+  //
+  // And the slip it catches got *quieter*, not louder, on that path: the injected form
+  // is `try{(0,eval)(...)}catch(e){}`, so a parse failure inside the string is swallowed
+  // by that catch and costs the demo nothing visible — Tier-1 monitoring simply goes
+  // off, with no build error and no blank preview to notice it by. The HTML entry
+  // (Tier-2's only channel, `workers/api/src/monitor-inject.ts`) still inlines the body,
+  // where a slip is loud. This assertion is the only thing that fails first.
   assert.doesNotThrow(() => acorn.parse(REPORTER_SOURCE, { ecmaVersion: 5 }));
 });