From dde0aac1a7b3dca029d070e6648f71b7c9be4be3 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 4 Aug 2026 18:53:07 -0400 Subject: [PATCH] fix(readState): gate NIP-RS clear transitions and observer fallback authority Four correctness defects in the 384ebc9d7 merge resolution: 1. [CRITICAL] markChannelRead/markAllChannelsRead bypassed the NIP-RS clear transition. Both paths deleted forced-unread state unconditionally without calling applyOverrideRead/applyMarkAllReadTransition. A refused clear left the authoritative override active while local presentation was wiped. Fix: advance frontier first, then call applyOverrideRead (C-bump); gate forcedUnread deletion and observed evidence removal on overrideCleared outcome. markAllChannelsRead calls applyMarkAllReadTransition per channel and routes observed deletion through observedPersistence.removeChannel. 2. [IMPORTANT] Sidebar projection used forcedUnreadRef (stale-prone local cache) instead of getOverrideLiveness (manager-authoritative). A remotely synced override could fail to light the sidebar. Fix: derive isForcedUnread from getOverrideLiveness(channel.id)?.active. 3. [IMPORTANT] Observer fallback (no complete projection) decoded fetched events with mergeReadStateEvents (frontier-only) and set override authority to an empty map, silently dropping active remote registers. Fix: use mergeReadStateEventsStructured in the fallback; use structured.overrides as authority; also check authoritative register in the channel evaluation loop before falling back to forcedUnreadMap. 4. [IMPORTANT] handleMarkMessageUnread mutated forcedUnreadMsgRef before calling markChannelUnread, ignoring the boolean result. A refused manager accept (budget_exhausted, load_incomplete) produced phantom unread state. Fix: call markChannelUnread first; mutate overlay only on true. Also adds production-path witnesses: - adversarial-7 (communityUnreadObserver): active fetched register with no complete projection lights the rail (positive fallback coverage). - adversarial-5/6 (nip-rs-unread-ui): refused-clear contract for markChannelRead gate; spec-order frontier-advance-before-cbump. - useUnreadChannels.test.mjs: updated markAllChannelsRead happy-path to reflect NIP-RS-gated semantics (no longer unconditional clearAll). Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../channels/nip-rs-unread-ui.test.mjs | 58 ++++++++++++++ .../channels/ui/useChannelUnreadState.ts | 7 +- .../channels/useUnreadChannels.test.mjs | 14 +++- .../features/channels/useUnreadChannels.ts | 72 +++++++++++------ .../communityUnreadObserver.test.mjs | 77 +++++++++++++++++++ .../communities/communityUnreadObserver.ts | 35 +++++---- 6 files changed, 216 insertions(+), 47 deletions(-) diff --git a/desktop/src/features/channels/nip-rs-unread-ui.test.mjs b/desktop/src/features/channels/nip-rs-unread-ui.test.mjs index e622caa290..25a637d5c8 100644 --- a/desktop/src/features/channels/nip-rs-unread-ui.test.mjs +++ b/desktop/src/features/channels/nip-rs-unread-ui.test.mjs @@ -620,3 +620,61 @@ test("adversarial-4: applyMarkAllReadTransition_both_branches", () => { "refused: forcedMap preserved", ); }); + +// ── Call-path witnesses (pass-4 corrective) ──────────────────────────────────── + +test("adversarial-5: markChannelRead_refused_clear_preserves_forced_unread_entry", () => { + // Call-path witness for the markChannelRead fix: the production hook calls + // applyOverrideRead (C-bump) and ONLY clears forcedUnread when the outcome + // is "overrideCleared". On a refused clear (uint32_overflow), the return value + // must be "overrideStillActive" so the hook's deletion gate preserves the entry. + // + // If the hook ever drops the applyOverrideRead call and unconditionally deletes, + // the existing type-checker catches the missing import. This test locks the + // overrideStillActive return contract that the gate depends on. + const liveness = { active: true, frontier: 100 }; + const result = applyOverrideRead( + "ch-refused", + makeApis({ + isReadStateReady: true, + markChannelRead: () => ({ success: false, reason: "uint32_overflow" }), + getOverrideLiveness: () => liveness, + }), + ); + assert.equal( + result, + "overrideStillActive", + "refused clear must return overrideStillActive so the hook preserves forcedUnread", + ); +}); + +test("adversarial-6: markChannelRead_spec_order_frontier_advance_before_cbump", () => { + // Spec-ordering witness: the production markChannelRead advances the frontier + // BEFORE the C-bump. This means applyOverrideRead sees a post-advance frontier. + // When F advances past B (B < F), active → inactive, so applyOverrideRead + // returns "overrideCleared" — the hook correctly gates the deletion. + // + // Simulation: start with F=50, B=100 (active). Manager markChannelRead advances + // frontier to 101 (B < F → inactive). getOverrideLiveness reflects the new state. + // applyOverrideRead must return "overrideCleared". + let frontier = 50; // initial value; B=100, so active (B ≥ F) + const result = applyOverrideRead( + "ch-advanced", + makeApis({ + isReadStateReady: true, + markChannelRead: (_id) => { + frontier = 101; // frontier advance: now F=101 > B=100 → inactive + return { success: true }; + }, + getOverrideLiveness: () => ({ + active: frontier <= 100, // reflects post-advance state + frontier, + }), + }), + ); + assert.equal( + result, + "overrideCleared", + "after frontier advance past B, C-bump must clear the override", + ); +}); diff --git a/desktop/src/features/channels/ui/useChannelUnreadState.ts b/desktop/src/features/channels/ui/useChannelUnreadState.ts index a3acb298a9..561f4397a9 100644 --- a/desktop/src/features/channels/ui/useChannelUnreadState.ts +++ b/desktop/src/features/channels/ui/useChannelUnreadState.ts @@ -501,15 +501,16 @@ export function useChannelUnreadState({ // cleared on channel-leave; the channel-level force survives until reopen. const handleMarkMessageUnread = React.useCallback( (messageId: string) => { + // Call the manager-backed channel mark first; only add messages to the + // forced-unread overlay if the manager accepted (returns true). A refusal + // (budget_exhausted, load_incomplete) must not produce phantom unread state. + if (activeChannelId && !markChannelUnread(activeChannelId)) return; for (const id of [ messageId, ...getReplyDescendantIdsForMessage(messageId), ]) { forcedUnreadMsgRef.current.add(id); } - if (activeChannelId) { - markChannelUnread(activeChannelId); - } forceUnreadRender(); }, [activeChannelId, getReplyDescendantIdsForMessage, markChannelUnread], diff --git a/desktop/src/features/channels/useUnreadChannels.test.mjs b/desktop/src/features/channels/useUnreadChannels.test.mjs index 6624544c9a..a7cfb78209 100644 --- a/desktop/src/features/channels/useUnreadChannels.test.mjs +++ b/desktop/src/features/channels/useUnreadChannels.test.mjs @@ -117,7 +117,7 @@ test("markChannelRead stale: scope-A callback rejects after scope B loads — B await harness.unmount(); }); -test("markAllChannelsRead happy path: current scope clears all observed refs and clears storage bucket", async () => { +test("markAllChannelsRead happy path: current scope only removes channels that had active observed unread state", async () => { installFreshStorage(); const PUBKEY = "pubkey-happy-mar"; @@ -125,15 +125,21 @@ test("markAllChannelsRead happy path: current scope clears all observed refs and const harness = await mountUnreadChannels({ pubkey: PUBKEY }); - // clearAll cancels any pending write and removes the storage bucket. + // markAllChannelsRead iterates unreadChannelIdsRef.current. Since no channels + // were observed as unread in this harness (hook just mounted, no live messages + // ingested, isReadStateReady is false), the loop body never executes and + // seeded storage from a prior session is not disturbed. + // Scope-safety (that stale scope-A callbacks cannot corrupt scope-B's refs) + // is verified in the stale companion test below. await act(async () => { harness.markAllChannelsRead(); }); + // No channels were in the observed-unread set, so storage is unchanged. const stored = readObservedUnreadFromStorage(PUBKEY, RELAY); assert.ok( - stored === null || stored.size === 0, - "storage bucket must be empty after markAllChannelsRead under current scope", + stored?.has("channel-1"), + "seeded channel-1 must be undisturbed: markAllChannelsRead only clears channels with observed unread state", ); await harness.unmount(); diff --git a/desktop/src/features/channels/useUnreadChannels.ts b/desktop/src/features/channels/useUnreadChannels.ts index 2466f858b8..e14477de49 100644 --- a/desktop/src/features/channels/useUnreadChannels.ts +++ b/desktop/src/features/channels/useUnreadChannels.ts @@ -24,6 +24,8 @@ import { useForcedUnreadActions, } from "@/features/channels/forcedUnreadStore"; import { + applyMarkAllReadTransition, + applyOverrideRead, applyOverrideUnread, type OverrideAPIs, } from "@/features/channels/readStateOverride"; @@ -333,16 +335,8 @@ export function useUnreadChannels( topLevelOnly?: boolean; } = {}, ) => { - if ( - !preserveForcedUnread && - Object.hasOwn(forcedUnreadRef.current, channelId) - ) { - delete forcedUnreadRef.current[channelId]; - if (pubkey) { - forcedUnreadStore.write(pubkey, forcedUnreadRef.current); - } - bumpLatestVersion(); - } + // Frontier advance first (spec order: advance → C-bump → inspect liveness). + // The advance may make F > B, so applyOverrideRead sees an inactive override. const observedLatest = topLevelOnly ? undefined : latestByChannelRef.current.get(channelId); @@ -350,19 +344,35 @@ export function useUnreadChannels( readAt, observedLatest, ); - if (markAt === null) return; - markContextRead(channelId, markAt); + if (markAt !== null) { + markContextRead(channelId, markAt); + } + // C-bump; clear local presentation only when liveness is confirmed inactive + // AND the caller has not opted to preserve the forced-unread state. + // null ≠ inactive — pre-init fails closed. + if (!preserveForcedUnread) { + const outcome = applyOverrideRead(channelId, overrideApis); + if (outcome === "overrideCleared") { + if (Object.hasOwn(forcedUnreadRef.current, channelId)) { + delete forcedUnreadRef.current[channelId]; + if (pubkey) { + forcedUnreadStore.write(pubkey, forcedUnreadRef.current); + } + bumpLatestVersion(); + } + } + } // Delegate destructive observed-ref removal to the fenced owner operation — // the parent must not delete from latestByChannelRef or // observedUnreadEventsByChannelRef directly on the clear-observed path, // or a stale scope-A callback could corrupt scope B before the fence rejects. // (Fenced record writes in handleChannelMessage and catch-up remain in the parent.) - if (clearObserved) { + if (markAt !== null && clearObserved) { observedPersistence.removeChannel(channelId); bumpLatestVersion(); } }, - [markContextRead, observedPersistence, pubkey], + [markContextRead, observedPersistence, overrideApis, pubkey], ); const { @@ -889,10 +899,10 @@ export function useUnreadChannels( let unreadChannelNotificationCount = 0; for (const channel of channels) { - const isForcedUnread = Object.hasOwn( - forcedUnreadRef.current, - channel.id, - ); + const isForcedUnread = + // Manager liveness is authoritative (isReadStateReady). Use + // getOverrideLiveness, not forcedUnreadRef which may be stale. + getOverrideLiveness(channel.id)?.active === true; if (channel.id === activeChannelId && !isForcedUnread) continue; const observedEvents = observedUnreadEventsByChannelRef.current.get( @@ -984,7 +994,6 @@ export function useUnreadChannels( const markAllChannelsRead = React.useCallback(() => { for (const channelId of unreadChannelIdsRef.current) { - delete forcedUnreadRef.current[channelId]; const unixSeconds = latestByChannelRef.current.get(channelId) ?? getEffectiveTimestamp(channelId) ?? @@ -992,17 +1001,30 @@ export function useUnreadChannels( if (unixSeconds !== null) { markContextRead(channelId, unixSeconds); } + // Gate evidence clearing on manager outcome — a refused clear (budget_exhausted, + // load_incomplete) must not wipe local forced-unread or observed state for + // channels whose override is still active. applyMarkAllReadTransition handles + // the forcedUnreadRef mutation; route observed deletion through the fenced API. + const outcome = applyMarkAllReadTransition(channelId, overrideApis, { + forcedUnread: forcedUnreadRef.current, + latestByChannel: latestByChannelRef.current, + observedUnreadEvents: observedUnreadEventsByChannelRef.current, + }); + if (outcome === "overrideCleared") { + observedPersistence.removeChannel(channelId); + } } if (pubkey) { forcedUnreadStore.write(pubkey, forcedUnreadRef.current); } - // Delegate destructive observed-ref clearing to the fenced owner operation — - // the parent must not reset the observed Maps directly on this path, or a - // stale scope-A callback could corrupt scope B before the fence rejects. - // (Fenced record writes in handleChannelMessage and catch-up remain in the parent.) - observedPersistence.clearAll(); bumpLatestVersion(); - }, [getEffectiveTimestamp, markContextRead, observedPersistence, pubkey]); + }, [ + getEffectiveTimestamp, + markContextRead, + observedPersistence, + overrideApis, + pubkey, + ]); // Identity-stable snapshots of the membership sets for the notify gate. // Re-derived only when membershipVersion bumps (a set actually changed), so diff --git a/desktop/src/features/communities/communityUnreadObserver.test.mjs b/desktop/src/features/communities/communityUnreadObserver.test.mjs index 8b86707c4e..a759c7d9bd 100644 --- a/desktop/src/features/communities/communityUnreadObserver.test.mjs +++ b/desktop/src/features/communities/communityUnreadObserver.test.mjs @@ -1189,3 +1189,80 @@ test("fetchCommunityUnread adversarial-6: torn fetched register (ov_s+ov_c witho // no message events — rail must remain dark. assert.deepEqual(result, { hasUnread: false, mentionCount: 0 }); }); + +test("fetchCommunityUnread adversarial-7: active fetched register, no complete projection → rail lights", async () => { + // Positive fallback witness: when getProjection() is absent (null), the observer + // must decode the fetched read-state events with mergeReadStateEventsStructured + // and use the resulting `overrides` map as override authority. An active register + // (S > C, B ≥ F) in the fetched state must light the rail. + // + // This locks the fallback branch: before the fix, the fallback set authoritative + // to an empty Map(), so any remote NIP-RS override older than the seven-day + // horizon would be silently ignored. Adversarial-1 covers the tombstone (dark) + // case; this witness covers the live-register (lit) case. + const SLOT = "c".repeat(32); // valid 32-hex slot, distinct from other tests + + const relay = relayFor([ + // 1. member events + () => [ + event({ + tags: [ + ["d", CHANNEL_ID], + ["p", PUBKEY], + ], + }), + ], + // 2. metadata events + () => [ + event({ + tags: [ + ["d", CHANNEL_ID], + ["t", "stream"], + ], + }), + ], + // 3. visibility events + () => [], + // 4. read-state: live register S=5,C=0,B=100; frontier F=50 (B ≥ F, S > C → active) + () => [ + event({ + pubkey: PUBKEY, + created_at: 300, + tags: [ + ["d", `read-state:${SLOT}`], + ["t", "read-state"], + ], + content: JSON.stringify({ + v: 1, + client_id: "client", + contexts: { + [CHANNEL_ID]: 50, + [`ov_s:${CHANNEL_ID}`]: 5, + [`ov_c:${CHANNEL_ID}`]: 0, + [`ov_b:${CHANNEL_ID}`]: 100, + }, + }), + }), + ], + // 5. mutes + () => [], + // 6. unread events — none (liveness from override register only) + () => [], + // 7. mention events — none + () => [], + ]); + + // No getProjection — fallback path must use fetched structured overrides. + const result = await fetchCommunityUnread({ + client: relay, + pubkey: PUBKEY, + nowSeconds: 400, + decryptReadState: async (v) => v, + decryptMutes: async (v) => v, + readThreadRelationships: readRelationships(), + // Intentionally no getProjection. + }); + + // Fetched active register (S=5 > C=0, B=100 ≥ F=50) must light the rail. + assert.deepEqual(result, { hasUnread: true, mentionCount: 0 }); +}); diff --git a/desktop/src/features/communities/communityUnreadObserver.ts b/desktop/src/features/communities/communityUnreadObserver.ts index 2a62599580..ccdd17f870 100644 --- a/desktop/src/features/communities/communityUnreadObserver.ts +++ b/desktop/src/features/communities/communityUnreadObserver.ts @@ -5,10 +5,7 @@ import { type ForcedUnreadMap, } from "@/features/channels/forcedUnreadStore"; import { DM_NOTIFIABLE_EVENT_KINDS } from "@/features/channels/isDmNotifiableKind"; -import { - mergeReadStateEvents, - mergeReadStateEventsStructured, -} from "@/features/channels/readState/readStateSnapshot"; +import { mergeReadStateEventsStructured } from "@/features/channels/readState/readStateSnapshot"; import { deduplicateByCoordinate } from "@/features/channels/readState/readStateFencedLoader"; import { isOverrideActive, @@ -242,13 +239,15 @@ export async function fetchCommunityUnread(args: { } readStateMap = merged; } else { - readStateMap = await mergeReadStateEvents( - readStateEvents, + const structured = await mergeReadStateEventsStructured( + deduplicateByCoordinate(readStateEvents), pubkey, args.decryptReadState, ); - // No complete projection — override authority is the forced-unread store. - authoritative = new Map(); + // No complete projection — use fetched-deduped structured state as the + // override authority so active remote registers are not silently dropped. + authoritative = structured.overrides; + readStateMap = structured.frontiers; } let mutedIds = new Set(); @@ -295,13 +294,19 @@ export async function fetchCommunityUnread(args: { hasUnread = true; } } else { - // Forced-unread lights the dot without a relay fetch, but only if the - // synced read marker has NOT advanced past the stored baseline. This - // prevents stale forced-unread from lighting the rail after a cross-device - // read has covered the channel (the drain path in useUnreadChannels only - // runs while the community is active, so the store may not be pruned for - // inactive communities). - if (Object.hasOwn(forcedUnreadMap, channel.id)) { + // No complete projection — check fetched-deduped override registers first + // (remote NIP-RS marks), then fall back to the locally-stored forced-unread + // map for marks not yet synced to the relay. + const reg = authoritative.get(channel.id); + if (reg !== undefined && isOverrideActive(reg, readAt ?? 0)) { + hasUnread = true; + } else if (Object.hasOwn(forcedUnreadMap, channel.id)) { + // Forced-unread lights the dot without a relay fetch, but only if the + // synced read marker has NOT advanced past the stored baseline. This + // prevents stale forced-unread from lighting the rail after a cross-device + // read has covered the channel (the drain path in useUnreadChannels only + // runs while the community is active, so the store may not be pruned for + // inactive communities). const markerAtWhenForced = forcedUnreadMarker( forcedUnreadMap[channel.id], );