Fix confirmed findings from the #389 adversarial review

src/main/incident/AppRunJournal.ts

@@ -139,8 +139,18 @@ export class AppRunJournal {
     // we swallow and continue instead.
     try {
       if (typeof process.report === 'object' && process.report !== null) {
-        process.report.reportOnFatalError = true
+        // Directory FIRST, flag second. In the reverse order, a throw from
+        // the directory setter (sandbox profile that permits the boolean but
+        // not the string, path-validation quirk) would leave
+        // reportOnFatalError=true with Node's default directory ('' → CWD) —
+        // a real OOM would then write report.*.json somewhere the prior-run
+        // classifier never looks, silently reverting the crash to the
+        // force_quit_or_power_loss fallback. With directory first, a throw
+        // means the flag never gets set: no report anywhere beats a report
+        // in the wrong place, because the classifier's minidump path still
+        // catches the crash.
         process.report.directory = this.runDir
+        process.report.reportOnFatalError = true
       }
     } catch (err) {
       console.warn('[incident-journal] could not enable process.report:', err)
@@ -332,6 +342,16 @@ export class AppRunJournal {
     this.eventLoopDelay.disable()
     // Synchronous: the async flush() never lands during quit teardown (see flushSync).
     this.flushSync()
+    // Latch closed. Without this, record() kept accepting events after stop()
+    // — and long-lived async consumers (the debug-retention prune promise
+    // chain holds this journal as its sink for the whole process lifetime)
+    // can legitimately fire a `.then` in the residual event-loop window
+    // between will-quit returning and process teardown. Those events landed
+    // in `pending` with every drain timer already cleared: never written,
+    // pure dead weight. stop() is only called on the quit path (after
+    // markCleanShutdown) and on fatal startup failure, so there is no
+    // legitimate post-stop record() to preserve.
+    this.started = false
   }
 
   markCleanShutdown(reason: string): void {

src/main/incident/previousRunClassifier.ts

@@ -91,6 +91,15 @@ export function classifyPreviousRun(
     // terminated by V8 before JS could react. OOM is by far the common case; a
     // future observed non-OOM FatalError can be split out here if needed.
     classification = 'main_oom_suspected'
+    // A V8 fatal abort raises SIGABRT, so Crashpad almost always wrote a
+    // minidump for the SAME crash the diagnostic report describes. Look it up
+    // here too: when this branch first landed it returned without touching
+    // minidumpPath, which REGRESSED evidence for the exact crash class the
+    // branch was added for — pre-report-flag code fell through to the
+    // minidump lookup below and surfaced `native`/`minidumpPath` for OOM
+    // aborts, the new branch dropped both, and downstream triage that
+    // symbolicates from context.minidumpPath got undefined.
+    minidumpPath = findRecentMinidump(options.crashDumpsDir, readPriorStartedAt(priorRunDir))
   } else if (kinds.has('main.uncaught_exception')) {
     classification = 'main_crash_suspected'
   } else if (
@@ -150,21 +159,31 @@ export function classifyPreviousRun(
 //
 // WHY only reads the `trigger` field: the report can be tens of megabytes of
 // heap statistics + stacks. Parsing it fully during a synchronous startup scan
-// would defeat the "cheap classifier" invariant. The trigger is a top-level
-// string; JSON.parse on a bounded prefix is enough to read it. The full report
-// stays on disk for triage tools to read at leisure.
+// would defeat the "cheap classifier" invariant. `trigger` lives under the
+// report's `header` object (NOT at the top level — the shape is
+// `{ "header": { "trigger": "OOMError", ... }, "javascriptStack": ..., ... }`),
+// but `header` is the FIRST key Node serializes, so the field reliably lands
+// in the first few hundred bytes of the file — far inside the 32 KB prefix.
+// The full report stays on disk for triage tools to read at leisure.
 function findNodeDiagnosticReport(
   priorRunDir: string,
 ): { path: string; trigger: string | undefined } | undefined {
   let candidates: string[]
   try {
+    // Match Node's actual report name shape (report.<YYYYMMDD>.<HHMMSS>.
+    // <PID>.<TID>.<SEQ>.json) instead of a loose report.*.json prefix test.
+    // The loose test had a shadowing hazard: any future artifact named e.g.
+    // `report.summary.json` sorts lexically AFTER the digit-dated names,
+    // wins the "newest" pick below, has no trigger field — and the real
+    // fatal-error report sitting right next to it is never read, silently
+    // reverting the classification to force_quit_or_power_loss.
     candidates = readdirSync(priorRunDir)
-      .filter(name => name.startsWith('report.') && name.endsWith('.json'))
+      .filter(name => /^report\.\d{8}\.\d{6}\.\d+\.\d+\.\d+\.json$/.test(name))
   } catch {
     return undefined
   }
   if (candidates.length === 0) return undefined
-  // Report names contain an ISO-like timestamp so lexical sort ~= chronological.
+  // Report names embed date+time first, so lexical sort ~= chronological.
   candidates.sort()
   const reportPath = join(priorRunDir, candidates[candidates.length - 1])
   return { path: reportPath, trigger: readNodeReportTrigger(reportPath) }
@@ -187,17 +206,31 @@ function readNodeReportTrigger(reportPath: string): string | undefined {
     const fd = openSync(reportPath, 'r')
     try {
       const buf = Buffer.alloc(readBytes)
-      readSync(fd, buf, 0, readBytes, 0)
-      text = buf.toString('utf8')
+      // Slice to the byte count readSync ACTUALLY returned. Discarding the
+      // return value and stringifying the whole zero-initialized buffer
+      // padded a short read with NUL characters — harmless to the regex
+      // itself, but it made the scan lie about how much of the file was
+      // really inspected. Short reads on a settled regular file are rare
+      // (EINTR, racing truncation) but cost one line to handle honestly.
+      const bytesRead = readSync(fd, buf, 0, readBytes, 0)
+      text = buf.toString('utf8', 0, bytesRead)
     } finally {
       closeSync(fd)
     }
   } catch {
     return undefined
   }
-  // Node writes trigger as a top-level string field: `"trigger": "OOMError"` etc.
-  // A quick regex is safer than trying to close a truncated JSON object with a
-  // real parser; if the prefix is malformed, we just return undefined.
+  // `trigger` is nested under `header` in a Node diagnostic report; this regex
+  // works because it is a FLAT text scan over the prefix, not a structural
+  // lookup — nesting is irrelevant to it. Do NOT "simplify" this to
+  // `JSON.parse(prefix).trigger`: that returns undefined (trigger is not
+  // top-level) and would silently kill every main_oom_suspected
+  // classification. The flat scan is also safe from false positives in
+  // practice: `header` is the first serialized object and its `trigger` comes
+  // before any user-controlled content (commandLine/env land later in the
+  // header, stacks later still), so the FIRST match in the file is always the
+  // real one. A regex also survives the truncated-JSON prefix a real parser
+  // would reject.
   const match = /"trigger"\s*:\s*"([^"\\]+)"/.exec(text)
   return match?.[1]
 }

src/main/index.ts

@@ -195,6 +195,20 @@ async function startApp(): Promise<void> {
     perfEnabled: performanceService.getConfig().enabled,
     lock,
   })
+  // Wire the retention journal sink BEFORE start(): start() itself fires the
+  // run's FIRST prune (scheduleDebugStoragePrune('incident-run-start') at the
+  // end of AppRunJournal.start), and the prune's completion handler reads the
+  // module-level sink. With the sink set after `await appRunJournal.start()`,
+  // journaling that first prune was only race-free by ACCIDENT — the prune's
+  // internal statfs() I/O happened to resolve later than the synchronous
+  // continuation that set the sink, and any future `await` inserted between
+  // start() and the wiring would have silently dropped the first prune's
+  // journal entry. Setting the sink first makes the ordering structural
+  // instead of incidental. Safe to do pre-start: setDebugRetentionJournal
+  // just stores the reference, and AppRunJournal.record() no-ops until
+  // started. See #388 — the July 2026 crash forensics lost the retention
+  // breadcrumb entirely because pruning was console-only.
+  setDebugRetentionJournal(appRunJournal)
   await appRunJournal.start()
   appRunJournal.record({
     area: 'state.lock',
@@ -215,13 +229,6 @@ async function startApp(): Promise<void> {
   })
   installWindowIncidentHooks(appRunJournal)
   orchestrationBridge.setJournal(appRunJournal)
-  // Give debug-retention a handle to the journal so its pruning actions land in
-  // events.jsonl instead of only console.warn. Wired here — right after the
-  // journal starts and before the first scheduleDebugStoragePrune('startup')
-  // call below — so the very first prune is journal-visible. See #388: the
-  // July 2026 crash forensics lost the retention breadcrumb entirely because it
-  // was console-only.
-  setDebugRetentionJournal(appRunJournal)
   try {
     // Native crashes (V8 aborts, SIGSEGV in native addons, GPU-process death)
     // never reach JS, so the JSONL hooks above cannot see them. Crashpad writes

src/main/performance/heapWatchdog.ts

@@ -49,11 +49,24 @@ import { getAppRunId } from '@main/incident/appRunIds.js'
 // Current 1.5 GiB ceiling: verified from the crashed run's own
 // PerformanceService trajectory — 34 hours of samples oscillated in the
 // 30–400 MB band even under heavy multi-agent load. 1.5 GiB is ~3x that
-// steady-state ceiling, so under normal use it stays quiet, but it now
-// covers BOTH failure modes above:
-//   - the 1.2 GB gradual-climb OOM (was already covered by the ratio),
-//   - the 2.5 GB burst OOM (previously missed because it never reached
-//     the fixed ceiling).
+// steady-state ceiling, so under normal use it stays quiet, and it now
+// catches the 2.5 GB burst OOM (previously missed because it never
+// reached the 3 GiB fixed ceiling).
+//
+// HONEST COVERAGE NOTE — the ~1.2 GB abort class is still NOT caught by
+// polling. On the 2026-05-11 host (heapLimit ~4.29 GiB) the ratio arm
+// resolves to 0.70 × 4.29 ≈ 3.0 GiB and the fixed arm is 1.5 GiB, so a
+// process that aborts around 1.2 GB never crosses either. An earlier
+// revision of this comment claimed the ratio covered that case; it never
+// did (min(3 GiB, 3 GiB) = 3 GiB pre-change, min(1.5, 3) = 1.5 GiB now —
+// both above 1.2 GB). Lowering the trip under ~1 GiB to chase it would
+// fire during legitimate heavy sessions (the 34 h trace above peaked
+// ~400 MB, but bundle builds and giant history loads spike past 1 GiB
+// briefly). Those low-peak aborts are instead attributed after the fact
+// by process.report.reportOnFatalError + the prior-run classifier
+// (main_oom_suspected, PR #389): no snapshot, but a diagnostic report
+// with heap stats and stacks. Polling buys a PRE-crash snapshot only for
+// climbs that cross ~1.5 GiB.
 //
 // Snapshot writes remain single-shot per run (see snapshotWritten
 // below), so lowering the ceiling doesn't create repeated snapshot