Fix the review findings from PR #1056

Greptile and CodeRabbit between them found eight things, all verified against
the code before changing anything.

The renderer-recovery event reported the attempt, not the outcome. It emitted
`recovered: true` the moment the budget allowed a reload, so a reload that was
attempted and then failed still went out as a recovery. It now reports after
`loadURL` settles — true on resolve, false on reject, and false when the budget
refuses outright — so the metric describes what the user got rather than what
ADE tried.

Compaction could be skipped entirely by a payload it could not measure. When
`JSON.stringify` throws (a BigInt anywhere in the object), the fallback measured
`String(value)` — "[object Object]", 15 bytes, under every cap — so the original
unbounded payload was stored and sent untouched, which is the one case a cap
exists for. Unmeasurable is now treated as must-compact. A circular reference
never reached this path; inline-image redaction breaks the cycle first.

Compaction also ran once per subscriber. `sendChatEvent` compacted inside the
per-peer loop, so one live event serialized and binary-searched its payload
again for every peer watching that session. It is memoized against the envelope
now — once per event, no matter how many peers.

The shortened-diff matcher was unanchored, on both desktop and iOS, so a real
diff whose own changed lines quoted the notice strings — editing the compactor,
for instance — was classified as compacted and reported as zero additions and
deletions. Both now require the header at the start.

iOS measured the suspension gap on the wall clock, the same defect already fixed
on the desktop budget: a device whose clock moves backward during a long
suspension would report a short or negative gap and go on to trust a socket iOS
had already suspended. It uses `systemUptime` now. And a shortened diff no longer
draws a `-0` deletion badge through the delete-kind branch, which contradicted
the VoiceOver label beside it.

One test weakness: `bytes(undefined)` is 0, so upper-bound assertions about
`structured` passed just as happily if compaction had deleted the field instead
of bounding it. Size claims about fields that must survive now go through a
helper that also asserts a lower bound.

Regression tests: reload-rejection reporting, the unserializable payload, the
anchored matcher on both platforms, and the fenced-content case from the earlier
commit.

Author: arul28

apps/ade-cli/src/services/sync/syncHostService.ts

@@ -5379,9 +5379,24 @@ export function createSyncHostService(args: SyncHostServiceArgs) {
     }
   }
 
+  /**
+   * Compaction serializes the payload and binary-searches it, so doing it per
+   * peer meant one live event paid that cost once for every subscriber. The
+   * result depends only on the envelope, so it is memoized against the envelope
+   * identity and computed once per event no matter how many peers receive it.
+   */
+  const compactedSyncEnvelopes = new WeakMap<AgentChatEventEnvelope, AgentChatEventEnvelope>();
+  function compactChatEventEnvelopeOnce(event: AgentChatEventEnvelope): AgentChatEventEnvelope {
+    const cached = compactedSyncEnvelopes.get(event);
+    if (cached) return cached;
+    const compacted = compactChatEventEnvelopeForSync(event);
+    compactedSyncEnvelopes.set(event, compacted);
+    return compacted;
+  }
+
   function sendChatEvent(peer: PeerState, event: AgentChatEventEnvelope, seq: number): "sent" | "already-sent" | "failed" {
     if (chatEventAlreadySent(peer, event)) return "already-sent";
-    const syncEvent = compactChatEventEnvelopeForSync(event);
+    const syncEvent = compactChatEventEnvelopeOnce(event);
     const sent = send(peer.ws, "chat_event", { ...syncEvent, seq } satisfies SyncChatEventPayload);
     if (sent) markChatEventSent(peer, event);
     return sent ? "sent" : "failed";

apps/desktop/src/main/main.ts

@@ -807,16 +807,17 @@ async function createWindow(args: {
     // Monotonic on purpose: the budget is a rolling time window, and a wall-clock
     // correction mid-crash-storm would either free the budget early or freeze it.
     const decision = rendererRecoveryBudget.requestAttempt(details.reason, performance.now());
-    // A lost renderer is a product-level failure category, so it is reported
-    // once per occurrence with Electron's own closed reason enum and whether the
-    // retry budget still allowed a reload. The budget bounds the volume: a
-    // boot-crash loop stops trying, so it cannot emit forever.
-    if (isRecoverableRenderProcessGone(details.reason)) {
-      args.onRendererRecovery?.({
-        crash_reason: coarseRenderProcessGoneReason(details.reason),
-        recovered: decision.recover,
-      });
-    }
+    // A lost renderer is a product-level failure category, reported once per
+    // occurrence with Electron's own closed reason enum. `recovered` describes
+    // the OUTCOME, not the intent: a reload that was attempted and then failed
+    // is a window that stayed down, and reporting it as recovered would make the
+    // metric describe our try rather than the user's result. The budget bounds
+    // the volume — a boot-crash loop stops trying, so it cannot emit forever.
+    const crashReason = coarseRenderProcessGoneReason(details.reason);
+    const reportRecovery = (recovered: boolean): void => {
+      if (!isRecoverableRenderProcessGone(details.reason)) return;
+      args.onRendererRecovery?.({ crash_reason: crashReason, recovered });
+    };
     if (!decision.recover) {
       if (decision.cause === "budget-exhausted") {
         args.logger?.error("window.render_process_recovery_abandoned", {
@@ -825,6 +826,7 @@ async function createWindow(args: {
           attempts: decision.attempts,
           windowMs: RENDERER_RECOVERY_WINDOW_MS,
         });
+        reportRecovery(false);
       }
       return;
     }
@@ -842,14 +844,18 @@ async function createWindow(args: {
       // Load the canonical renderer URL rather than reloading whatever was last
       // committed: a crash on the load-failure fallback page would otherwise
       // just reload the error page.
-      win.loadURL(recoveryUrl).catch((error) => {
-        args.logger?.error("window.render_process_recovery_failed", {
-          windowId: win.id,
-          reason: details.reason,
-          attempt,
-          err: toErrorMessage(error),
-        });
-      });
+      win.loadURL(recoveryUrl).then(
+        () => reportRecovery(true),
+        (error) => {
+          args.logger?.error("window.render_process_recovery_failed", {
+            windowId: win.id,
+            reason: details.reason,
+            attempt,
+            err: toErrorMessage(error),
+          });
+          reportRecovery(false);
+        },
+      );
     }, RENDERER_RECOVERY_DELAY_MS);
   });
 

apps/desktop/src/renderer/components/chat/chatTranscriptRows.test.ts

@@ -1116,6 +1116,20 @@ describe("summarizeDiffStats", () => {
     expect(summarizeDiffStats(diff)).toEqual({ additions: 0, deletions: 0 });
   });
 
+  it("does not treat a normal diff containing shortening notices as compacted", () => {
+    // Editing the compactor (or this matcher) produces a real diff whose own
+    // added lines quote both notice strings. An unanchored search called that
+    // change compacted and reported it as zero additions and deletions.
+    const diff = [
+      "@@ -1,4 +1,6 @@",
+      '+    `[ADE] Large ${label} was shortened to keep this chat fast.`,',
+      '+    `[ADE] ${omittedBytes} bytes were left out.`,',
+      "-  const old = true;",
+    ].join("\n");
+
+    expect(summarizeDiffStats(diff)).toEqual({ additions: 2, deletions: 1 });
+  });
+
   /**
    * The notice became user-facing when phones started receiving the same
    * compacted events, so its wording changed. The case above pins the old text

apps/desktop/src/renderer/components/chat/chatTranscriptRows.ts

@@ -652,7 +652,11 @@ export function summarizeDiffStats(diff: string): { additions: number; deletions
   // same compaction feeds phones, so it can no longer talk about "stored chat
   // history"), but transcripts already on disk carry the old text and must keep
   // being recognized as shortened rather than counted as real diff lines.
-  const shortenedDiff = diff.includes("[ADE] Large file diff was shortened")
+  // Anchored at the start: the compactor always emits this header as the first
+  // line. An unanchored search matched a real diff whose own changed lines
+  // quoted the notice — editing this file, for instance — and reported that
+  // change as having no additions or deletions.
+  const shortenedDiff = diff.startsWith("[ADE] Large file diff was shortened")
     && (diff.includes("bytes were left out.") || diff.includes("bytes omitted from stored chat history."));
   if (shortenedDiff) {
     return { additions: 0, deletions: 0 };

apps/desktop/src/shared/chatEventCompaction.test.ts

@@ -15,6 +15,17 @@ function toolResult(overrides: Partial<Extract<AgentChatEvent, { type: "tool_res
 
 const bytes = (value: unknown) => Buffer.byteLength(JSON.stringify(value) ?? "", "utf8");
 
+/**
+ * `bytes(undefined)` is 0, so an upper-bound assertion alone passes just as
+ * happily when a field was deleted as when it was bounded. Every size claim
+ * about a field that must SURVIVE compaction goes through here.
+ */
+function expectBoundedNotDropped(value: unknown, maxBytes: number) {
+  expect(value).toBeDefined();
+  expect(bytes(value)).toBeGreaterThan(0);
+  expect(bytes(value)).toBeLessThan(maxBytes);
+}
+
 describe("chat event compaction", () => {
   /**
    * The defect this module exists to prevent: `structured` was added to
@@ -27,8 +38,8 @@ describe("chat event compaction", () => {
 
     const stored = compactChatEventForStorage(event) as Extract<AgentChatEvent, { type: "tool_result" }>;
 
-    expect(bytes(stored.structured)).toBeLessThan(bytes(huge) / 10);
-    expect(bytes(stored.structured)).toBeLessThan(32 * 1024);
+    expectBoundedNotDropped(stored.structured, Math.floor(bytes(huge) / 10));
+    expectBoundedNotDropped(stored.structured, 32 * 1024);
   });
 
   it("leaves a small structured payload untouched", () => {
@@ -157,6 +168,26 @@ describe("chat event compaction", () => {
     expect(bytes(stored.result)).toBeLessThan(40 * 1024);
   });
 
+  it("bounds a payload that cannot be serialized at all", () => {
+    // A BigInt anywhere in the payload makes JSON.stringify throw, and the
+    // fallback measured "[object Object]" — 15 bytes, under every cap — so the
+    // original unbounded object was stored and sent untouched. (A circular
+    // reference does not reach here: inline-image redaction breaks the cycle
+    // first, after which the payload measures normally.)
+    const unserializable: Record<string, unknown> = {
+      rows: Array.from({ length: 5_000 }, (_, i) => `row ${i}`),
+      cursor: BigInt(42),
+    };
+
+    const stored = compactChatEventForStorage(
+      toolResult({ result: unserializable }),
+    ) as Extract<AgentChatEvent, { type: "tool_result" }>;
+
+    expect(stored.result).not.toBe(unserializable);
+    expect(bytes(stored.result)).toBeLessThan(4 * 1024);
+    expect(JSON.stringify(stored.result)).toContain("[ADE]");
+  });
+
   it("is idempotent on the text branches too", () => {
     // These carry no wrapper to recognize; they are stable only because the
     // compacted output lands under the cap. Shrinking that headroom would
@@ -205,7 +236,7 @@ describe("chat event compaction", () => {
 
     expect(stored.resultOriginalBytes).toBe(5_000_000);
     expect(stored.resultOmittedBytes).toBe(4_900_000);
-    expect(bytes(stored.structured)).toBeLessThan(32 * 1024);
+    expectBoundedNotDropped(stored.structured, 32 * 1024);
   });
 
   it("returns the same object when there is nothing to compact", () => {

apps/desktop/src/shared/chatEventCompaction.ts

@@ -200,14 +200,20 @@ const isCompactedPayloadWrapper = (value: unknown): boolean => {
     && typeof candidate.omittedBytes === "number";
 };
 
-const stringifyPayloadForCompaction = (value: unknown): { text: string; structured: boolean } => {
-  if (typeof value === "string") return { text: value, structured: false };
+const stringifyPayloadForCompaction = (
+  value: unknown,
+): { text: string; structured: boolean; serializable: boolean } => {
+  if (typeof value === "string") return { text: value, structured: false, serializable: true };
   try {
     const json = JSON.stringify(value, null, 2);
-    if (typeof json === "string") return { text: json, structured: true };
-    return { text: String(value), structured: false };
+    if (typeof json === "string") return { text: json, structured: true, serializable: true };
+    return { text: String(value), structured: false, serializable: true };
   } catch {
-    return { text: String(value), structured: false };
+    // A circular reference or a BigInt anywhere in the payload lands here, and
+    // `String(value)` is "[object Object]" — 15 bytes, under every cap. Reporting
+    // that as the size let the ORIGINAL unbounded object through untouched,
+    // which is the one case the cap exists for.
+    return { text: String(value), structured: false, serializable: false };
   }
 };
 
@@ -232,6 +238,16 @@ const compactStoredUnknownPayload = (
     return null;
   }
   const serialized = stringifyPayloadForCompaction(value);
+  if (!serialized.serializable) {
+    // Unmeasurable is not "small". Substitute a bounded placeholder rather than
+    // storing and transmitting a payload no cap could see.
+    const summary = `[ADE] Large ${label} could not be measured and was left out.`;
+    return {
+      value: { summary, originalBytes: 0, omittedBytes: 0, preview: serialized.text.slice(0, 256) },
+      originalBytes: 0,
+      omittedBytes: 0,
+    };
+  }
   const compacted = compactStoredTextPayload(label, serialized.text, maxBytes);
   if (!compacted) return null;
   if (!serialized.structured) {

apps/ios/ADE/Services/SyncService.swift

@@ -3781,8 +3781,12 @@ final class SyncService: ObservableObject {
   private var pendingRemoteProfileDbVersionBySite: [String: Int] = [:]
   private let discoveryBrowser = SyncBonjourBrowser()
   private var reconnectState = SyncReconnectState()
-  /// When the app last went to the background, used to classify the resume.
-  private var backgroundedAt: Date?
+  /// Uptime when the app last went to the background, used to classify the
+  /// resume. Deliberately monotonic (`systemUptime`), not wall clock: a device
+  /// whose clock moves backward during a long suspension would otherwise report
+  /// a short or negative gap and go on to trust a socket iOS had already
+  /// suspended — the exact failure this classifier exists to prevent.
+  private var backgroundedAtUptime: TimeInterval?
   private var envelopeChunkAssembler = SyncEnvelopeChunkAssembler()
   private var envelopeChunkExpiryTask: Task<Void, Never>?
   private var transportProbeTask: Task<Void, Never>?
@@ -7786,15 +7790,15 @@ final class SyncService: ObservableObject {
   /// long background to `.refreshOnly` and trust a socket iOS may already have
   /// suspended. The cheap error is the safe one.
   func handleBackgroundTransition() {
-    backgroundedAt = Date()
+    backgroundedAtUptime = ProcessInfo.processInfo.systemUptime
   }
 
   func handleForegroundTransition() async {
     refreshPhoneTailnetInterfaceState()
     let resumeAction = syncForegroundResumeAction(
-      backgroundGapSeconds: backgroundedAt.map { Date().timeIntervalSince($0) }
+      backgroundGapSeconds: backgroundedAtUptime.map { ProcessInfo.processInfo.systemUptime - $0 }
     )
-    backgroundedAt = nil
+    backgroundedAtUptime = nil
     // Coming to the foreground is new information: the user is here, and the
     // network may be a different one entirely. Reset the attempt ladder even
     // from the terminal unreachable state, which otherwise costs the user a

apps/ios/ADE/Views/Work/WorkChatRichCardViews.swift

@@ -999,6 +999,13 @@ struct WorkFileChangeCardView: View {
   /// counts are zero. VoiceOver would otherwise read the same zeros as a fact
   /// about the change ("0 additions, 0 deletions") instead of the absence of a
   /// measurement, so it gets the reason instead.
+  /// A shortened diff has no trustworthy counts, so neither badge is drawn —
+  /// the delete-kind branch would otherwise still render `-0` and contradict the
+  /// VoiceOver label right beside it.
+  private var showsChangeCounts: Bool {
+    !workDiffWasShortened(card.diff)
+  }
+
   private var changeCountDescription: String {
     workDiffWasShortened(card.diff)
       ? "Change counts unavailable, diff was shortened"
@@ -1038,12 +1045,12 @@ struct WorkFileChangeCardView: View {
 
           Spacer(minLength: 6)
 
-          if diffStats.additions > 0 {
+          if showsChangeCounts, diffStats.additions > 0 {
             Text("+\(diffStats.additions)")
               .font(.caption.monospaced())
               .foregroundStyle(ADEColor.success)
           }
-          if diffStats.deletions > 0 || card.kind.lowercased() == "delete" {
+          if showsChangeCounts, diffStats.deletions > 0 || card.kind.lowercased() == "delete" {
             Text("-\(diffStats.deletions)")
               .font(.caption.monospaced())
               .foregroundStyle(ADEColor.danger)

apps/ios/ADE/Views/Work/WorkEventMapping.swift

@@ -927,7 +927,9 @@ func nonEmpty(_ value: String) -> String? {
 /// chat history" to "to keep this chat fast" — but transcripts already on disk
 /// carry the old wording and must keep being recognized.
 func workDiffWasShortened(_ diff: String) -> Bool {
-  guard diff.contains("[ADE] Large file diff was shortened") else { return false }
+  // Anchored at the start, mirroring desktop: an unanchored search matched a
+  // real diff whose own changed lines quoted the notice.
+  guard diff.hasPrefix("[ADE] Large file diff was shortened") else { return false }
   return diff.contains("bytes were left out.")
     || diff.contains("bytes omitted from stored chat history.")
 }