Skip to content

fix(desktop): send only the durable row_id on rewind, not the ordinal - #90091

Closed
RichardGuan1 wants to merge 1 commit into
NousResearch:mainfrom
RichardGuan1:fix/desktop-rewind-send-rowid-only
Closed

RichardGuan1 wants to merge 1 commit into
NousResearch:mainfrom
RichardGuan1:fix/desktop-rewind-send-rowid-only

Conversation

@RichardGuan1

Copy link
Copy Markdown

Problem

Rewinding, editing, or regenerating a turn on a long-lived desktop session fails server-side
with:

truncate_before_user_ordinal (75) does not match truncate_before_row_id target turn (102)

(code 4030, from _reconcile_client_ordinal). The desktop sent all three truncation
addresses at once (truncate_before_user_ordinal, truncate_before_message_id,
truncate_before_row_id). The gateway resolves the row_id to its own durable ordinal and
refuses any mismatch — but across a long session the renderer's visible-user ordinal drifts
from the gateway's active-durable ordinal (the #87059 drift class; observed 7 → 27 in one
day), so the ordinal is wrong even though the row_id is correct, and every legitimate rewind
gets refused. Reproducible on current main (9 refusals on one session), independent of
context compression (prefix_user_count=0).

Fix

When a bound integer row_id is available, the desktop sends only truncate_before_row_id and
drops the redundant ordinal + message-id. This matches the gateway's documented contract —
prefer row_id over ordinals, and keep the ordinal only as a back-compat / optimistic-row path
when no durable id exists. Safety is unchanged: an unknown/dead row_id still fails closed with
4018, dropped turns are soft-archived (active=0) rather than deleted, and confirm_truncate
/ ordinal-0 confirm_empty_truncate gating is preserved.

Tests

Updated rewind.test.ts so a bound rowId plus a divergent ordinal sends row_id alone, and an
ordinal-0 restore still carries confirm_empty_truncate. rewind.test.ts 32 + index.test.tsx
122 pass (216 in use-prompt-actions/).

The renderer's user-ordinal can drift from the gateway's durable ordinal
(75 vs 102, NousResearch#87059), so a rewind carrying both a bound row_id and that
stale ordinal failed the gateway's 4030 cross-check even though the
row_id resolved fine. The gateway prefers row_id over ordinals, so send
the ordinal and message-id only when no row_id is available; a stale
row_id still fails closed (4018). Tests cover the row_id-only path and
ordinal-0 confirm_empty_truncate.
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 19, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review; please use your judgment.

Precise fix for a false-refusal class: when a bound integer row_id is in hand it's the authoritative address, so dropping the renderer-visible user ordinal and synthetic message-id eliminates the 4030 cross-check mismatch (observed 75 vs 102 divergence) without weakening safety — an unknown/dead row_id still fails closed with 4018, the Number.isInteger guard keeps malformed values on the old reconciling path, message-id-only addressing is untouched, and confirm_empty_truncate survives for ordinal-0 restores. All three behaviors have tests, and the previously pinned expectation was flipped with its rationale documented inline citing the gateway's own reconciliation comment.

No blocking issues found.

— reviewer-a · automated agent review (Hermes week-review)

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks — right diagnosis on the #87059 class: a bound durable row_id should aim the cut alone, and sending a divergent renderer ordinal alongside it tripped the gateway's 4030 cross-check into refusing legitimate rewinds.

This landed on main via PR #93784 (the composite-carrier retry/undo salvage, merged today): rewind.ts now sends only the durable row_id when one is bound, and the merged tests assert truncate_before_user_ordinal is undefined in that case. I verified by running this PR's own test file — including your new divergent-ordinal and ordinal-0 cases — against current main's implementation unmodified: 32/32 pass.

Closing as implemented on main. Appreciate the careful writeup — sorry the broader carrier rework got there first.

@teknium1 teknium1 closed this Aug 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants