fix(site-builder): stop the console-error marker from re-deriving "original" from a live wrapper

Review found a gap in the idempotency marker added for Task 16: if
external code wraps our patched console.error between two of our own
installs, the marker sees an unmarked function and treats it as
virgin, capturing the external wrapper itself as "the original".
That both double-records (the old patch is still reachable inside the
wrapper's closure) and makes __resetConsoleErrorBuffer() restore to
the wrapper instead of the real original.

Fix: stash the true original exactly once, directly on the `console`
object (not module scope, so it survives HMR too), and always
re-wrap that stashed reference rather than whatever console.error
currently is. Reinstalling after an external wrap now discards that
wrapper instead of guessing whether it still chains to us -- a
deliberate, documented trade-off, since there is no safe way to tell
those two cases apart from the outside.

Also: window error/rejection listeners now catch exceptions from a
hostile e.reason the same way the console.error patch already did,
and the module doc comment now notes the known HMR buffer-orphan
wrinkle. Adds two tests covering the external-wrapper and
module-re-execution scenarios; both were mutation-verified to fail
against the prior implementation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

craft/src/utils/console-buffer.test.ts

@@ -67,4 +67,53 @@ describe('console error buffer', () => {
     first.push({ ts: 0, message: 'injected' });
     expect(getRecentConsoleErrors()).toHaveLength(1);
   });
+
+  test('reinstalling after external code wraps console.error does not double-record, and reset restores the true original', () => {
+    const trueOriginal = vi.spyOn(console, 'error').mockImplementation(() => {});
+    installConsoleErrorBuffer();
+
+    // External code wraps our patch without knowing anything about our
+    // marker convention -- this is exactly what a third-party script or
+    // another monitor might do.
+    const ourPatch = console.error;
+    const external = vi.fn((...args: unknown[]) => {
+      ourPatch(...args);
+    });
+    console.error = external;
+
+    installConsoleErrorBuffer();
+    console.error('dup-check');
+
+    expect(getRecentConsoleErrors().filter((e) => e.message === 'dup-check')).toHaveLength(1);
+
+    __resetConsoleErrorBuffer();
+    expect(console.error).toBe(trueOriginal);
+  });
+
+  test('surviving module re-execution: reinstalling after the module reloads does not re-wrap an already-patched console.error', async () => {
+    vi.spyOn(console, 'error').mockImplementation(() => {});
+    const mod1 = await import('./console-buffer');
+    mod1.installConsoleErrorBuffer();
+    const patchedAfterFirstInstall = console.error;
+
+    // Simulate HMR: the module graph re-evaluates, producing a fresh module
+    // instance with its own reset top-level state, while the *global*
+    // console.error is still whatever the previous instance patched it to.
+    vi.resetModules();
+    const mod2 = await import('./console-buffer');
+    mod2.installConsoleErrorBuffer();
+
+    // The marker on the live console.error -- not module-local state -- is
+    // what installConsoleErrorBuffer() consults, so the reloaded instance
+    // must recognize the existing patch and leave it alone rather than
+    // wrapping it a second time.
+    expect(console.error).toBe(patchedAfterFirstInstall);
+
+    console.error('once-across-reload');
+    expect(
+      mod1.getRecentConsoleErrors().filter((e) => e.message === 'once-across-reload')
+    ).toHaveLength(1);
+
+    mod1.__resetConsoleErrorBuffer();
+  });
 });

craft/src/utils/console-buffer.ts

@@ -9,16 +9,61 @@
  * The console.error patch ALWAYS chains to the original. A monitor that
  * swallows diagnostics is worse than no monitor.
  *
- * Idempotency note: installConsoleErrorBuffer() is guarded by a marker
- * stamped directly on the patched console.error function itself, not just a
- * module-scoped boolean. React 18 StrictMode (which double-invokes effects)
- * and hot module reload can re-run this module's top-level code, resetting
- * any local `let installed = false` while the *global* console.error is
- * still the already-patched function from the previous install. A
- * boolean-only guard would then treat that already-patched function as "the
- * original" and wrap it again, building a chain that grows on every reload.
- * Checking the marker on the live console.error avoids that regardless of
- * how many times this module's top level re-executes.
+ * Idempotency and the "true original":
+ *
+ * `installConsoleErrorBuffer()` treats console.error's *live* MARKER stamp
+ * (not module-scoped state) as the source of truth for "am I already the
+ * active patch". That alone survives React 18 StrictMode / hot module
+ * reload re-running this module's top level while the previous instance's
+ * patch is still installed on the global -- a plain `let installed = false`
+ * would reset on re-execution and treat that already-patched function as
+ * virgin, wrapping it again and building a chain that grows on every
+ * reload.
+ *
+ * The marker alone isn't sufficient once something *other* than us has
+ * touched console.error since our last install, though. If external code
+ * wraps our patch (`console.error = L` where L internally calls our patched
+ * function), console.error is unmarked again from our point of view, so a
+ * naive "unmarked == virgin" re-install would capture L -- an intermediate
+ * wrapper, not the true original -- as "the original" to chain to. That
+ * would (a) leave our OLD patch still reachable inside L's closure, so one
+ * console.error() call records twice (once via the new patch, once via the
+ * old one still buried inside L), and (b) make __resetConsoleErrorBuffer()
+ * restore console.error to L instead of the real original, permanently
+ * losing the reference to it.
+ *
+ * The fix is to never re-derive "the original" from whatever the live
+ * console.error happens to be at install time. Instead, the true original
+ * is captured exactly once and stashed as a hidden property directly on the
+ * `console` object (not in module scope, so it also survives module
+ * re-execution) the first time we ever patch. Every subsequent install,
+ * whether triggered by our own idempotent re-install, HMR, or a reinstall
+ * after external code has wrapped or replaced console.error, reuses that
+ * stashed reference and re-wraps it directly -- guaranteeing exactly one
+ * patch layer chains straight to the real original, and that reset can
+ * always find it.
+ *
+ * Trade-off this implies: if install() is called again after some external
+ * code has wrapped console.error, that external wrapper is discarded (we
+ * re-wrap the true original directly, not the external wrapper) rather than
+ * preserved. We accept that over the alternative of chaining through an
+ * unknown wrapper, which cannot be done safely -- there is no way to detect
+ * whether that wrapper still calls through to our old patch (risking double
+ * recording if we also wrap it) or has fully replaced it (risking losing
+ * capture entirely if we don't). An external wrapper installed *after* us
+ * and left alone (i.e. install() is not called again) is completely
+ * unaffected -- it just sits on top of our patch and both continue to work
+ * as normal JS monkey-patch layering.
+ *
+ * Known residual gap (not fixed, documented instead): if this module is hot
+ * reloaded while console.error stays patched from the previous instance,
+ * the live patch's closure still points at the *previous* module
+ * instance's `buffer` array. The marker check correctly stops the new
+ * instance from re-wrapping, but that also means the new instance's own
+ * `getRecentConsoleErrors()` reads its own (empty) buffer forever while
+ * capture silently continues into the orphaned previous instance's buffer.
+ * This only matters across an actual HMR reload during development --
+ * production has exactly one module instance for the lifetime of the page.
  */
 
 export interface ConsoleErrorEntry {
@@ -29,16 +74,29 @@ export interface ConsoleErrorEntry {
 const MAX_ENTRIES = 20;
 const MAX_MESSAGE = 500;
 const MARKER = '__whpConsoleErrorBufferPatched';
+const TRUE_ORIGINAL_KEY = '__whpConsoleErrorBufferTrueOriginal';
 
-type MarkedConsoleError = typeof console.error & {
-  [MARKER]?: true;
-  __original?: typeof console.error;
-};
+type MarkedConsoleError = typeof console.error & { [MARKER]?: true };
+type ConsoleWithStash = typeof console & { [TRUE_ORIGINAL_KEY]?: typeof console.error };
 
 function isPatched(fn: typeof console.error): fn is MarkedConsoleError {
   return typeof fn === 'function' && (fn as MarkedConsoleError)[MARKER] === true;
 }
 
+function getTrueOriginal(): typeof console.error | undefined {
+  return (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY];
+}
+
+function setTrueOriginal(fn: typeof console.error): void {
+  (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY] = fn;
+}
+
+function clearTrueOriginal(): void {
+  delete (console as ConsoleWithStash)[TRUE_ORIGINAL_KEY];
+}
+
+// Module-scoped: not consulted for correctness (see doc comment above), only
+// used to avoid re-adding window listeners within a single module instance.
 let buffer: ConsoleErrorEntry[] = [];
 let errorListener: ((e: ErrorEvent) => void) | null = null;
 let rejectionListener: ((e: PromiseRejectionEvent) => void) | null = null;
@@ -61,25 +119,42 @@ function stringifyArg(arg: unknown): string {
 
 /** Idempotent. Patches console.error and adds window error listeners. */
 export function installConsoleErrorBuffer(): void {
+  // We're already the live, active patch -- nothing to do.
   if (isPatched(console.error)) return;
 
-  const original = console.error.bind(console);
+  // Reuse the stashed true original if we've ever patched before (covers
+  // HMR re-execution and reinstall-after-external-wrap); otherwise this is
+  // a genuinely virgin install and the current console.error IS the true
+  // original.
+  const trueOriginal = getTrueOriginal() ?? console.error;
+  setTrueOriginal(trueOriginal);
+
   const patched: MarkedConsoleError = (...args: unknown[]): void => {
     try {
       record(args.map(stringifyArg).join(' '));
     } catch {
       // Recording must never break logging.
     }
-    original(...args);
+    trueOriginal.call(console, ...args);
   };
   patched[MARKER] = true;
-  patched.__original = original;
   console.error = patched;
 
-  if (typeof window !== 'undefined') {
-    errorListener = (e: ErrorEvent) => record(`window.onerror: ${e.message}`);
-    rejectionListener = (e: PromiseRejectionEvent) =>
+  if (typeof window !== 'undefined' && !errorListener) {
+    errorListener = (e: ErrorEvent) => {
+      try {
+        record(`window.onerror: ${e.message}`);
+      } catch {
+        // Recording must never break the page's own error handling.
+      }
+    };
+    rejectionListener = (e: PromiseRejectionEvent) => {
+      try {
         record(`unhandledrejection: ${stringifyArg(e.reason)}`);
+      } catch {
+        // Recording must never break the page's own rejection handling.
+      }
+    };
     window.addEventListener('error', errorListener);
     window.addEventListener('unhandledrejection', rejectionListener);
   }
@@ -93,10 +168,11 @@ export function getRecentConsoleErrors(): ConsoleErrorEntry[] {
 /** Test-only: clear the buffer and un-patch. */
 export function __resetConsoleErrorBuffer(): void {
   buffer = [];
-  const current = console.error;
-  if (isPatched(current) && current.__original) {
-    console.error = current.__original;
+  const trueOriginal = getTrueOriginal();
+  if (trueOriginal) {
+    console.error = trueOriginal;
   }
+  clearTrueOriginal();
   if (typeof window !== 'undefined') {
     if (errorListener) window.removeEventListener('error', errorListener);
     if (rejectionListener) window.removeEventListener('unhandledrejection', rejectionListener);