Skip to content

fix(desktop): role-aware tail-first stamping (re-land of #364) - #398

Merged
Kyzcreig merged 2 commits into
mainfrom
wt/role-aware-stamp
Jul 18, 2026
Merged

Kyzcreig merged 2 commits into
mainfrom
wt/role-aware-stamp

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

What

Re-lands #364 (role-aware tail-first stamping) onto current main. The original branch fell 705 commits behind; its hook file conflicted with the livesync Phase-1 work landed since.

The live mechanism (CDP-attach, 2026-07-15): a stale optimistic assistant row — its completion frame severed by a backend restart — absorbed the fresh turn's user_id via the old top-down role-blind walk. The zombie then wore a committed id (invisible to the #361 sweep) and painted its stale text as a permanent duplicate.

Both LIVE Greptile findings adjudicated (one fixed-by-documentation, one fixed-by-hardening)

  1. Silent user-id drop when no user-role stampable row exists — this is deliberate, now documented in-code and test-pinned: falling back to any-role would stamp the USER id onto an assistant row, re-creating the exact cross-role mis-stamp this PR exists to prevent. An unstamped-but-committed id is safe (the poll reconciles it with no optimistic twin to duplicate); a cross-role-stamped zombie is not.
  2. 3+-id frames (message_ids array + scalar fields both populated) — now handled by the same assistant-preferring tail-first walk: no crash, extras left for the poll, each id/row claimed at most once. Test-pinned.

Verification

  • RED-proven: swapping the old fork/main impl back in (keeping the new tests) fails 5/36 — the 3 zombie vectors + both edge-frame tests.
  • Hook suite: 36/36 green.
  • Full desktop suite delta vs clean fork/main baseline in an identical worktree: zero new failures (17 pre-existing reds on both trees — local-env; the 1 extra on my run passes 3/3 in isolation = flake).
  • tsc --noEmit: identical 7 pre-existing errors on both trees.

#364 will be closed as superseded with credit (Co-authored-by).

@greptile-apps

greptile-apps Bot commented Jul 18, 2026 •

Copy link
Copy Markdown

Greptile Summary

This PR re-lands the role-aware tail-first stamping algorithm (stampOptimisticTranscriptRows) that prevents stale assistant zombie rows from absorbing committed IDs during turn completion. Instead of the old linear role-blind walk, IDs are now claimed from the tail of the stampable list by matching role — user IDs go only to user rows, assistant IDs only to assistant rows — so zombies from a severed prior turn keep their optimistic IDs and remain sweepable.

  • The core 2-id path correctly prevents cross-role mis-stamping and is well-tested with the zombie scenario described in the live incident (2026-07-15).
  • The 3+-id extras loop (for frames where array and scalar fields are both populated) does not deduplicate committedIds before iterating; a duplicate ID from extractCommittedMessageIds can slip through and stamp a zombie with a committed ID that is already assigned to the user row, recreating the unsweepable-zombie duplicate-display bug in that specific combination.

Confidence Score: 4/5

The 2-id stamping path — the primary fix for the live zombie mis-stamp incident — is correct and well-tested. The 3+-id extras loop has a gap: when extractCommittedMessageIds returns duplicate IDs (array + scalar fields overlap) and a zombie row is unclaimed, the zombie can absorb a committed ID already assigned to the user row, recreating the unsweepable duplicate-display bug.

The core role-aware tail-first algorithm works correctly for the 2-id case covering the documented live incident. The 3+-id extras branch introduces a scenario where duplicate IDs from overlapping array and scalar fields can propagate a committed ID onto a zombie row — the exact failure mode the PR is hardening against. Deduplicating committedIds before branching would close the gap with minimal risk.

apps/desktop/src/app/chat/hooks/use-session-changes.ts — specifically the 3+-id extras loop at lines 428-432 and the absence of deduplication on committedIds before the length-based branch.

Important Files Changed

Filename Overview
apps/desktop/src/app/chat/hooks/use-session-changes.ts Replaces the linear role-blind stamp walk with a role-aware tail-first algorithm; the 3+-id extras loop can stamp a zombie with a duplicate committed ID when extractCommittedMessageIds returns overlapping array+scalar ids
apps/desktop/src/app/chat/hooks/use-session-changes.test.ts Adds two new describe blocks (9 tests) covering the zombie mis-stamp and edge-frame scenarios; the 3+-id test uses distinct ids without a zombie row, leaving the duplicate-id + zombie combination untested

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[stampOptimisticTranscriptRows called] --> B{committedIds.length?}
    B -- 0 --> C[Return unchanged messages]
    B -- 1 --> D[Lone-id path: prefer assistant tail, else any]
    B -- 2 --> E[2-id positional path]
    B -- 3+ --> F[3+-id path]

    E --> E1["claim(ids[0], 'user')"]
    E1 --> E2["claim(ids[1], 'assistant')"]
    E2 --> G[Build final messages via assignment map]

    F --> F1["claim(ids[0], 'user')"]
    F1 --> F2["claim(ids[1], 'assistant')"]
    F2 --> F3["for each extra id: claim assistant else any"]
    F3 -- "duplicate id + unclaimed zombie" --> F4["Zombie stamped with duplicate committed id — unsweepable"]
    F3 -- "all stampable rows claimed" --> G

    D --> D1["claim(id, 'assistant')"]
    D1 -- fail --> D2["claim(id, null)"]
    D1 -- success --> G
    D2 --> G

    G --> H[Return messages + stampedIds]
    H --> I[Poll: dropZombieOptimisticRows sweeps remaining optimistic rows]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[stampOptimisticTranscriptRows called] --> B{committedIds.length?}
    B -- 0 --> C[Return unchanged messages]
    B -- 1 --> D[Lone-id path: prefer assistant tail, else any]
    B -- 2 --> E[2-id positional path]
    B -- 3+ --> F[3+-id path]

    E --> E1["claim(ids[0], 'user')"]
    E1 --> E2["claim(ids[1], 'assistant')"]
    E2 --> G[Build final messages via assignment map]

    F --> F1["claim(ids[0], 'user')"]
    F1 --> F2["claim(ids[1], 'assistant')"]
    F2 --> F3["for each extra id: claim assistant else any"]
    F3 -- "duplicate id + unclaimed zombie" --> F4["Zombie stamped with duplicate committed id — unsweepable"]
    F3 -- "all stampable rows claimed" --> G

    D --> D1["claim(id, 'assistant')"]
    D1 -- fail --> D2["claim(id, null)"]
    D1 -- success --> G
    D2 --> G

    G --> H[Return messages + stampedIds]
    H --> I[Poll: dropZombieOptimisticRows sweeps remaining optimistic rows]
Loading

Reviews (2): Last reviewed commit: "fix(desktop): 3+-id frames keep the posi..." | Re-trigger Greptile

Comment thread apps/desktop/src/app/chat/hooks/use-session-changes.ts
Apollo and others added 2 commits July 18, 2026 12:36
… ids onto stale zombies (re-land of #364)

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>
@Kyzcreig
Kyzcreig force-pushed the wt/role-aware-stamp branch from f15ab4d to e0702d6 Compare July 18, 2026 19:36
@Kyzcreig
Kyzcreig merged commit e665670 into main Jul 18, 2026
27 checks passed
@Kyzcreig
Kyzcreig deleted the wt/role-aware-stamp branch July 18, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant