From 3286acb5742561e526b6ee72714a319d62192db9 Mon Sep 17 00:00:00 2001 From: Apollo Date: Sat, 18 Jul 2026 12:08:10 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(desktop):=20role-aware=20tail-first=20s?= =?UTF-8?q?tamping=20=E2=80=94=20stop=20stamping=20the=20turn=20ids=20onto?= =?UTF-8?q?=20stale=20zombies=20(re-land=20of=20#364)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-lands #364 (author: Kyzcreig; branch was 705 commits behind) onto current main. CDP-attach caught the mechanism live 2026-07-15: a stale optimistic assistant row (completion frame severed by a backend restart) absorbed the fresh turn user_id via the old top-down role-blind walk — the zombie wore a committed id (invisible to the #361 sweep) and painted as a permanent duplicate. stampOptimisticTranscriptRows now assigns role-aware and tail-first. Both LIVE Greptile #364 findings adjudicated: (1) silent user-id drop when no user-role stampable row exists is DELIBERATE and now documented + test-pinned — cross-role fallback would re-create the exact mis-stamp this fixes; the poll reconciles the committed row safely. (2) 3+-id frames (array + scalar fields both populated) now documented + test-pinned: assistant-preferring tail-first walk, extras left for the poll, no crash. RED-proven: swapping back the fork/main top-down impl fails 5/36 (the 3 zombie vectors + both edge-frame tests). Hook suite 36 green; full desktop suite delta vs clean fork/main baseline = zero new failures (17 pre-existing reds on both, 1 local-env flake passes 3/3 in isolation); tsc errors identical to baseline (7, all pre-existing). Co-authored-by: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com> --- .../chat/hooks/use-session-changes.test.ts | 89 +++++++++++++++++++ .../src/app/chat/hooks/use-session-changes.ts | 88 ++++++++++++++---- 2 files changed, 160 insertions(+), 17 deletions(-) diff --git a/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts b/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts index b82908a3ee7d9..e4529229cf209 100644 --- a/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts +++ b/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts @@ -645,3 +645,92 @@ describe('reconnect-seam zombie optimistic rows (severed message.complete stamp) expect(committed.messages.map(row => row.id)).toEqual(['700']) }) }) + +describe('role-aware tail-first stamping (stale zombie + fresh turn, live 2026-07-15)', () => { + function textMessage2(id: string, role: ChatMessage['role'], text: string, pending = false): ChatMessage { + return { id, role, pending, parts: [{ type: 'text', text }] } + } + + it('does NOT stamp a stale assistant zombie with the USER id (the mis-stamp that defeated the sweep)', () => { + // Transcript: [stale assistant zombie from a severed earlier turn, + // fresh user row, fresh completed assistant row]. Frame carries the fresh + // turn's [user_id, assistant_id]. The old top-down role-blind walk gave + // user_id -> zombie and assistant_id -> fresh user row: the zombie wore a + // committed id (unsweepable) and painted as a permanent duplicate. + const stale = textMessage2('assistant-stream-100', 'assistant', 'old severed reply') + const freshUser = textMessage2('user-200-abc', 'user', 'hi') + const freshAssistant = textMessage2('assistant-201', 'assistant', 'hello!') + + const stamped = stampOptimisticTranscriptRows([stale, freshUser, freshAssistant], ['500', '501']) + + // fresh rows get the ids; the stale zombie keeps its optimistic id + expect(stamped.messages.map(row => row.id)).toEqual(['assistant-stream-100', '500', '501']) + expect(stamped.messages[1].role).toBe('user') + expect(stamped.messages[2].role).toBe('assistant') + }) + + it('stale zombie left optimistic is then swept by the poll (end-to-end: no double paint)', () => { + const stale = textMessage2('assistant-stream-100', 'assistant', 'old severed reply') + const freshUser = textMessage2('user-200-abc', 'user', 'hi') + const freshAssistant = textMessage2('assistant-201', 'assistant', 'hello!') + + const stamped = stampOptimisticTranscriptRows([stale, freshUser, freshAssistant], ['500', '501']) + // poll returns the stale turn's committed rows + the fresh ones + const result = appendFetchedMessages(stamped.messages, [ + { id: 400, role: 'assistant', content: 'old severed reply' }, + { id: 500, role: 'user', content: 'hi' }, + { id: 501, role: 'assistant', content: 'hello!' } + ]) + + const texts = result.messages.map(row => row.parts.map(p => (p as { text?: string }).text ?? '').join('')) + expect(texts.filter(t => t === 'old severed reply')).toHaveLength(1) + expect(result.messages.map(row => row.id).sort()).toEqual(['400', '500', '501']) + }) + + it('normal send path unchanged: [user, assistant] stamps its own rows in order', () => { + const stamped = stampOptimisticTranscriptRows( + [textMessage2('user-1-x', 'user', 'q'), { ...textMessage2('assistant-2', 'assistant', 'a'), pending: false }], + ['100', '101'] + ) + expect(stamped.messages.map(row => row.id)).toEqual(['100', '101']) + }) + + it('lone id prefers the assistant streamed row over an older user leftover', () => { + const leftoverUser = textMessage2('user-9-z', 'user', 'unsent leftover') + const freshAssistant = textMessage2('assistant-10', 'assistant', 'reply') + const stamped = stampOptimisticTranscriptRows([leftoverUser, freshAssistant], ['700']) + + expect(stamped.messages.map(row => row.id)).toEqual(['user-9-z', '700']) + }) +}) + +describe('Greptile #364 edge frames', () => { + function tm(id: string, role: ChatMessage['role'], text: string, pending = false): ChatMessage { + return { id, role, pending, parts: [{ type: 'text', text }] } + } + + it('no user-role stampable row: user id is deliberately NOT cross-role stamped', () => { + // User row already committed; only the streamed assistant row + a stale + // assistant zombie remain. The user id must NOT land on either assistant + // row (cross-role mis-stamp); the assistant id lands on the FRESH tail row. + const zombie = tm('assistant-stream-old', 'assistant', 'stale') + const freshAssistant = tm('assistant-2', 'assistant', 'reply') + const stamped = stampOptimisticTranscriptRows([zombie, freshAssistant], ['500', '501']) + + expect(stamped.messages.map(row => row.id)).toEqual(['assistant-stream-old', '501']) + expect(stamped.stampedIds.has('500')).toBe(false) // user id dropped, poll reconciles + }) + + it('3+ ids (array + scalar fields both populated): no crash, tail-first assistant preference, extras unstamped', () => { + const user = tm('user-1-a', 'user', 'q') + const assistant = tm('assistant-2', 'assistant', 'a') + const stamped = stampOptimisticTranscriptRows([user, assistant], ['500', '501', '502']) + + // Two stampable rows, three ids: assistant-preferring walk stamps the + // assistant first, then any-role for the next id; the third id is left + // for the poll. No row is double-stamped, no id stamped twice. + expect(stamped.messages.map(row => row.id)).toEqual(['501', '500']) + expect(stamped.stampedIds.size).toBe(2) + expect(stamped.stampedIds.has('502')).toBe(false) + }) +}) diff --git a/apps/desktop/src/app/chat/hooks/use-session-changes.ts b/apps/desktop/src/app/chat/hooks/use-session-changes.ts index 4ce6875d85059..117e12c4b0391 100644 --- a/apps/desktop/src/app/chat/hooks/use-session-changes.ts +++ b/apps/desktop/src/app/chat/hooks/use-session-changes.ts @@ -359,30 +359,84 @@ export function stampOptimisticTranscriptRows(messages: readonly ChatMessage[], } } - const stampedIds = new Set() - let nextIdIndex = 0 + // Stampable = any non-committed-id row: optimistic id (`user-`/`assistant-` + // prefix) OR still-pending. The streamed assistant row is finalized by + // completeAssistantMessage() (pending:false, id `assistant-`) BEFORE + // markTurnComplete() runs this stamp — so a `pending`-only predicate would + // skip it (#352). + const stampable: number[] = [] + messages.forEach((message, index) => { + if (Number.isInteger(Number(message.id))) { + return + } + const isOptimisticId = message.id.startsWith('user-') || message.id.startsWith('assistant-') + if (isOptimisticId || message.pending) { + stampable.push(index) + } + }) - const next = messages.map(message => { - if (Number.isInteger(Number(message.id)) || nextIdIndex >= committedIds.length) { - return message + // ROLE-AWARE, TAIL-FIRST assignment. The frame's contract is + // [user_id, assistant_id] for the turn that JUST finished — but the list can + // also hold STALE optimistic rows from an earlier turn whose completion + // frame was severed by a backend restart (this session's live incident, + // 2026-07-15). The old top-down, role-blind walk stamped the USER id onto + // that stale ASSISTANT zombie and the ASSISTANT id onto the real user row — + // the zombie then wore a committed id (invisible to the #361 sweep) and + // painted its stale text as a permanent duplicate, while the current turn's + // rows stayed un-stamped. Assign each id to the LAST unclaimed stampable row + // of the matching role instead (a lone id keeps last-row-any-role), so stale + // zombies stay optimistic — and therefore sweepable. + const assignment = new Map() + const claimed = new Set() + const claim = (id: string, wantRole: 'user' | 'assistant' | null): boolean => { + for (let j = stampable.length - 1; j >= 0; j--) { + const index = stampable[j] + if (claimed.has(index)) { + continue + } + if (wantRole && messages[index].role !== wantRole) { + continue + } + assignment.set(index, id) + claimed.add(index) + return true } + return false + } + if (committedIds.length === 2) { + // [user_id, assistant_id] positional contract. A failed role-scoped claim + // deliberately DROPS the id (Greptile #364 flagged the silent discard — + // it is intentional, not a bug): if no user-role stampable row exists, + // the user row already committed, so the poll reconciles it by id with + // no optimistic twin to duplicate. Falling back to any-role here would + // stamp the USER id onto an assistant row — with a stale assistant + // zombie present, that re-creates the exact cross-role mis-stamp this + // function exists to prevent (zombie wears a committed id, unsweepable). + // Unstamped-but-committed is safe; cross-role-stamped is not. + claim(committedIds[0], 'user') + claim(committedIds[1], 'assistant') + } else { + // Non-two-id frames (a lone survivor, or 3+ ids when both an array field + // and scalar fields populate — Greptile #364). The positional [user, + // assistant] contract can't disambiguate here. For each id, prefer an + // assistant row (the dup-prone one — the poll always re-fetches the final + // text bubble), fall back to any unclaimed stampable row; ids beyond the + // stampable rows are simply not stamped (the poll reconciles them). + for (const id of committedIds) { + if (!claim(id, 'assistant')) { + claim(id, null) + } + } + } - // An optimistic row still needs stamping whether or not it is still - // `pending`. The streamed assistant row is finalized by - // completeAssistantMessage() (pending:false, id `assistant-`) BEFORE - // markTurnComplete() runs this stamp — so a `pending`-only predicate skips - // it, it keeps its optimistic id, and the post-completion poll re-appends - // the committed integer row as a DUPLICATE. Recognize any non-committed - // optimistic id (`user-`/`assistant-` prefix) OR a still-pending row. - const isOptimisticId = - message.id.startsWith('user-') || message.id.startsWith('assistant-') + const stampedIds = new Set() + const next = messages.map((message, index) => { + const id = assignment.get(index) - if (!isOptimisticId && !message.pending) { + if (id === undefined) { return message } - const id = committedIds[nextIdIndex] - nextIdIndex += 1 stampedIds.add(id) return { ...message, id, pending: false } From e0702d6d3daf82c8ed3e0845cbf0afa88668e03e Mon Sep 17 00:00:00 2001 From: Apollo Date: Sat, 18 Jul 2026 12:36:07 -0700 Subject: [PATCH 2/2] fix(desktop): 3+-id frames keep the positional [user, assistant] pair role-aware (Greptile #398) --- .../chat/hooks/use-session-changes.test.ts | 13 ++++++---- .../src/app/chat/hooks/use-session-changes.ts | 24 ++++++++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts b/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts index e4529229cf209..6039ef12047da 100644 --- a/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts +++ b/apps/desktop/src/app/chat/hooks/use-session-changes.test.ts @@ -721,15 +721,18 @@ describe('Greptile #364 edge frames', () => { expect(stamped.stampedIds.has('500')).toBe(false) // user id dropped, poll reconciles }) - it('3+ ids (array + scalar fields both populated): no crash, tail-first assistant preference, extras unstamped', () => { + it('3+ ids (array + scalar fields both populated): positional pair stays role-aware, extras unstamped', () => { const user = tm('user-1-a', 'user', 'q') const assistant = tm('assistant-2', 'assistant', 'a') const stamped = stampOptimisticTranscriptRows([user, assistant], ['500', '501', '502']) - // Two stampable rows, three ids: assistant-preferring walk stamps the - // assistant first, then any-role for the next id; the third id is left - // for the poll. No row is double-stamped, no id stamped twice. - expect(stamped.messages.map(row => row.id)).toEqual(['501', '500']) + // Greptile #398: the first two ids keep the [user_id, assistant_id] + // positional convention — 500 lands on the USER row, 501 on the assistant + // row (an assistant-preferring walk over all ids reversed them). The third + // id has no unclaimed row left and is left for the poll. + expect(stamped.messages.map(row => row.id)).toEqual(['500', '501']) + expect(stamped.messages[0].role).toBe('user') + expect(stamped.messages[1].role).toBe('assistant') expect(stamped.stampedIds.size).toBe(2) expect(stamped.stampedIds.has('502')).toBe(false) }) diff --git a/apps/desktop/src/app/chat/hooks/use-session-changes.ts b/apps/desktop/src/app/chat/hooks/use-session-changes.ts index 117e12c4b0391..f9ca36ee46257 100644 --- a/apps/desktop/src/app/chat/hooks/use-session-changes.ts +++ b/apps/desktop/src/app/chat/hooks/use-session-changes.ts @@ -415,13 +415,25 @@ export function stampOptimisticTranscriptRows(messages: readonly ChatMessage[], // Unstamped-but-committed is safe; cross-role-stamped is not. claim(committedIds[0], 'user') claim(committedIds[1], 'assistant') + } else if (committedIds.length > 2) { + // 3+ ids (array + scalar fields both populated — Greptile #364). The first + // two ids still follow the [user_id, assistant_id] positional convention, + // so claim them role-aware exactly like the 2-id case (Greptile #398: + // assistant-preferring EVERY id here put committedIds[0] — the user's id — + // onto an assistant row, a cross-role reversal). Only the EXTRAS beyond the + // positional pair fall to assistant-then-any; ids beyond the stampable rows + // are simply not stamped (the poll reconciles them). + claim(committedIds[0], 'user') + claim(committedIds[1], 'assistant') + for (const id of committedIds.slice(2)) { + if (!claim(id, 'assistant')) { + claim(id, null) + } + } } else { - // Non-two-id frames (a lone survivor, or 3+ ids when both an array field - // and scalar fields populate — Greptile #364). The positional [user, - // assistant] contract can't disambiguate here. For each id, prefer an - // assistant row (the dup-prone one — the poll always re-fetches the final - // text bubble), fall back to any unclaimed stampable row; ids beyond the - // stampable rows are simply not stamped (the poll reconciles them). + // Lone id: the positional contract can't disambiguate a single survivor. + // Prefer the streamed assistant row (the dup-prone one — the poll always + // re-fetches the final text bubble), fall back to any unclaimed row. for (const id of committedIds) { if (!claim(id, 'assistant')) { claim(id, null)