fix(sidebar): reject rev at MAX_SAFE_INTEGER boundary to keep mint headroom

The rev parse bound used Number.isSafeInteger, which accepts
Number.MAX_SAFE_INTEGER itself. The click path mints rev = maxRev + 1,
so an accepted boundary rev overflows to an unsafe value that never
advances again — recreating the same-second toggle wedge the bound
exists to prevent. Reject rev >= Number.MAX_SAFE_INTEGER (exclusive,
normalize to 0) so maxRev + 1 always stays safe. updatedAt keeps its
inclusive bound: the click path takes Math.max(now, observed), never
increments, so it cannot overflow.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src/features/sidebar/lib/channelMutesStorage.test.mjs

@@ -42,13 +42,15 @@ test("parseMutePayload: malformed rev (string / negative / non-integer / NaN / u
       neg: { muted: true, updatedAt: 1, rev: -2 },
       frac: { muted: true, updatedAt: 1, rev: 1.5 },
       nan: { muted: true, updatedAt: 1, rev: NaN },
-      // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a
-      // malformed entry at this magnitude would wedge later toggles forever
-      // unless rejected here (Carl P2).
+      // At or above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past
+      // Number.MAX_SAFE_INTEGER, so an entry at the boundary itself (or beyond)
+      // would wedge later toggles forever unless rejected here (Carl P2 /
+      // Thufir off-by-one). The bound is exclusive, so the boundary normalizes.
+      boundary: { muted: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER },
       huge: { muted: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 },
     },
   });
-  for (const id of ["str", "neg", "frac", "nan", "huge"]) {
+  for (const id of ["str", "neg", "frac", "nan", "boundary", "huge"]) {
     assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`);
     assert.equal(result.channels[id].muted, true, `${id} entry kept`);
   }
@@ -189,24 +191,48 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b
   );
 });
 
-test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => {
-  // Carl P2 regression. A malformed blob carries a same-second rev far above
-  // Number.MAX_SAFE_INTEGER on a `muted:true` entry. Before the parse bound,
-  // this survived as-is and — since a real later click mints rev = maxSeen + 1,
-  // which cannot advance past an unsafe integer — the true entry won the
-  // same-second tie forever, suppressing every later unmute. The parser now
-  // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins.
+test("mergeStores: a boundary (MAX_SAFE_INTEGER) rev cannot wedge later same-second toggles", () => {
+  // Carl P2 / Thufir off-by-one regression. A malformed blob carries a
+  // same-second rev of exactly Number.MAX_SAFE_INTEGER on a `muted:true`
+  // entry — the largest value the pre-fix `Number.isSafeInteger` guard still
+  // ACCEPTED. Had it survived, the click path (`rev = max(seen) + 1`) would
+  // mint Number.MAX_SAFE_INTEGER + 1, an unsafe integer that never advances,
+  // so the true entry would win every same-second tie forever. The parser now
+  // rejects `rev >= Number.MAX_SAFE_INTEGER` (exclusive), normalizing it to 0
+  // and preserving headroom for `maxRev + 1`. Drive the production mint
+  // (`Math.max(localRev, maxRevSeen) + 1`) through alternating same-second
+  // toggles and assert each later toggle mints a strictly-advancing rev and wins.
   const wedged = parseMutePayload({
     version: 1,
-    channels: { c: { muted: true, updatedAt: 100, rev: Number.MAX_VALUE } },
+    channels: {
+      c: { muted: true, updatedAt: 100, rev: Number.MAX_SAFE_INTEGER },
+    },
   });
-  assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0");
-  const laterUnmute = S(E(false, 100, 1));
-  assert.deepEqual(
-    mergeStores(wedged, laterUnmute).channels.c,
-    E(false, 100, 1),
-    "the same-second later unmute (rev 1) wins — no permanent wedge",
-  );
+  assert.equal(wedged.channels.c.rev, 0, "boundary rev is normalized to 0");
+
+  // Production mint: same fixed second (100), rev = max(local, seen) + 1.
+  const mint = (store, muted) => {
+    const local = store.channels.c;
+    const rev = Math.max(local?.rev ?? 0, 0) + 1;
+    return { store: S(E(muted, 100, rev)), rev };
+  };
+
+  let state = wedged;
+  let prevRev = state.channels.c.rev; // 0
+  for (const muted of [false, true, false, true]) {
+    const { store: click, rev } = mint(state, muted);
+    assert.ok(
+      rev > prevRev,
+      `mint rev ${rev} strictly advances past ${prevRev}`,
+    );
+    state = mergeStores(state, click);
+    assert.deepEqual(
+      state.channels.c,
+      E(muted, 100, rev),
+      `same-second toggle to ${muted} (rev ${rev}) wins — no wedge`,
+    );
+    prevRev = rev;
+  }
 });
 
 test("mergeStores: unmute with higher updatedAt overrides mute", () => {

desktop/src/features/sidebar/lib/channelMutesStorage.ts

@@ -56,19 +56,22 @@ export function parseMutePayload(json: unknown): ChannelMuteStore | null {
                 ((v as Record<string, unknown>).updatedAt as number) >= 0
               );
             })
-            // Normalize `rev`: accept a safe non-negative integer, otherwise 0.
-            // An entry is never dropped solely because `rev` is absent (older
-            // build) or malformed — absence is a valid mergeable value. `rev`
-            // above `Number.MAX_SAFE_INTEGER` is rejected here: at that
-            // magnitude `maxRev + 1` no longer advances, so a malformed entry
-            // could wedge every later toggle. Treating it as rev 0 lets a real
-            // later click win the same-second tie instead (Carl P2).
+            // Normalize `rev`: accept a non-negative integer with headroom for
+            // one mint, otherwise 0. An entry is never dropped solely because
+            // `rev` is absent (older build) or malformed — absence is a valid
+            // mergeable value. `rev >= Number.MAX_SAFE_INTEGER` is rejected:
+            // the click path mints `maxRev + 1`, so accepting the boundary
+            // itself would overflow to the same unsafe value and stop
+            // advancing, wedging every later toggle. Requiring strictly-below
+            // keeps `maxRev + 1` safe. Treating an out-of-range value as rev 0
+            // lets a real later click win the same-second tie instead (Carl P2).
             .map(([id, v]) => {
               const rawRev = v.rev;
               const rev =
                 typeof rawRev === "number" &&
                 Number.isSafeInteger(rawRev) &&
-                rawRev >= 0
+                rawRev >= 0 &&
+                rawRev < Number.MAX_SAFE_INTEGER
                   ? rawRev
                   : 0;
               return [

desktop/src/features/sidebar/lib/channelStarsStorage.test.mjs

@@ -42,13 +42,15 @@ test("parseStarPayload: malformed rev (string / negative / non-integer / NaN / u
       neg: { starred: true, updatedAt: 1, rev: -2 },
       frac: { starred: true, updatedAt: 1, rev: 1.5 },
       nan: { starred: true, updatedAt: 1, rev: NaN },
-      // Above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past it, so a
-      // malformed entry at this magnitude would wedge later toggles forever
-      // unless rejected here (Carl P2).
+      // At or above Number.MAX_SAFE_INTEGER: `maxRev + 1` cannot advance past
+      // Number.MAX_SAFE_INTEGER, so an entry at the boundary itself (or beyond)
+      // would wedge later toggles forever unless rejected here (Carl P2 /
+      // Thufir off-by-one). The bound is exclusive, so the boundary normalizes.
+      boundary: { starred: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER },
       huge: { starred: true, updatedAt: 1, rev: Number.MAX_SAFE_INTEGER + 1 },
     },
   });
-  for (const id of ["str", "neg", "frac", "nan", "huge"]) {
+  for (const id of ["str", "neg", "frac", "nan", "boundary", "huge"]) {
     assert.equal(result.channels[id].rev, 0, `${id} rev normalized to 0`);
     assert.equal(result.channels[id].starred, true, `${id} entry kept`);
   }
@@ -189,24 +191,48 @@ test("mergeStores: same-second old-build click (rev 0) loses to an earlier new-b
   );
 });
 
-test("mergeStores: a huge (unsafe) rev entry cannot wedge a later false toggle", () => {
-  // Carl P2 regression. A malformed blob carries a same-second rev far above
-  // Number.MAX_SAFE_INTEGER on a `starred:true` entry. Before the parse bound,
-  // this survived as-is and — since a real later click mints rev = maxSeen + 1,
-  // which cannot advance past an unsafe integer — the true entry won the
-  // same-second tie forever, suppressing every later unstar. The parser now
-  // normalizes the unsafe rev to 0, so a same-second later click (rev 1) wins.
+test("mergeStores: a boundary (MAX_SAFE_INTEGER) rev cannot wedge later same-second toggles", () => {
+  // Carl P2 / Thufir off-by-one regression. A malformed blob carries a
+  // same-second rev of exactly Number.MAX_SAFE_INTEGER on a `starred:true`
+  // entry — the largest value the pre-fix `Number.isSafeInteger` guard still
+  // ACCEPTED. Had it survived, the click path (`rev = max(seen) + 1`) would
+  // mint Number.MAX_SAFE_INTEGER + 1, an unsafe integer that never advances,
+  // so the true entry would win every same-second tie forever. The parser now
+  // rejects `rev >= Number.MAX_SAFE_INTEGER` (exclusive), normalizing it to 0
+  // and preserving headroom for `maxRev + 1`. Drive the production mint
+  // (`Math.max(localRev, maxRevSeen) + 1`) through alternating same-second
+  // toggles and assert each later toggle mints a strictly-advancing rev and wins.
   const wedged = parseStarPayload({
     version: 1,
-    channels: { c: { starred: true, updatedAt: 100, rev: Number.MAX_VALUE } },
+    channels: {
+      c: { starred: true, updatedAt: 100, rev: Number.MAX_SAFE_INTEGER },
+    },
   });
-  assert.equal(wedged.channels.c.rev, 0, "unsafe rev is normalized to 0");
-  const laterUnstar = S(E(false, 100, 1));
-  assert.deepEqual(
-    mergeStores(wedged, laterUnstar).channels.c,
-    E(false, 100, 1),
-    "the same-second later unstar (rev 1) wins — no permanent wedge",
-  );
+  assert.equal(wedged.channels.c.rev, 0, "boundary rev is normalized to 0");
+
+  // Production mint: same fixed second (100), rev = max(local, seen) + 1.
+  const mint = (store, starred) => {
+    const local = store.channels.c;
+    const rev = Math.max(local?.rev ?? 0, 0) + 1;
+    return { store: S(E(starred, 100, rev)), rev };
+  };
+
+  let state = wedged;
+  let prevRev = state.channels.c.rev; // 0
+  for (const starred of [false, true, false, true]) {
+    const { store: click, rev } = mint(state, starred);
+    assert.ok(
+      rev > prevRev,
+      `mint rev ${rev} strictly advances past ${prevRev}`,
+    );
+    state = mergeStores(state, click);
+    assert.deepEqual(
+      state.channels.c,
+      E(starred, 100, rev),
+      `same-second toggle to ${starred} (rev ${rev}) wins — no wedge`,
+    );
+    prevRev = rev;
+  }
 });
 
 test("mergeStores: unstar with higher updatedAt overrides star", () => {

desktop/src/features/sidebar/lib/channelStarsStorage.ts

@@ -56,19 +56,22 @@ export function parseStarPayload(json: unknown): ChannelStarStore | null {
                 ((v as Record<string, unknown>).updatedAt as number) >= 0
               );
             })
-            // Normalize `rev`: accept a safe non-negative integer, otherwise 0.
-            // An entry is never dropped solely because `rev` is absent (older
-            // build) or malformed — absence is a valid mergeable value. `rev`
-            // above `Number.MAX_SAFE_INTEGER` is rejected here: at that
-            // magnitude `maxRev + 1` no longer advances, so a malformed entry
-            // could wedge every later toggle. Treating it as rev 0 lets a real
-            // later click win the same-second tie instead (Carl P2).
+            // Normalize `rev`: accept a non-negative integer with headroom for
+            // one mint, otherwise 0. An entry is never dropped solely because
+            // `rev` is absent (older build) or malformed — absence is a valid
+            // mergeable value. `rev >= Number.MAX_SAFE_INTEGER` is rejected:
+            // the click path mints `maxRev + 1`, so accepting the boundary
+            // itself would overflow to the same unsafe value and stop
+            // advancing, wedging every later toggle. Requiring strictly-below
+            // keeps `maxRev + 1` safe. Treating an out-of-range value as rev 0
+            // lets a real later click win the same-second tie instead (Carl P2).
             .map(([id, v]) => {
               const rawRev = v.rev;
               const rev =
                 typeof rawRev === "number" &&
                 Number.isSafeInteger(rawRev) &&
-                rawRev >= 0
+                rawRev >= 0 &&
+                rawRev < Number.MAX_SAFE_INTEGER
                   ? rawRev
                   : 0;
               return [