fix(tui): exclude timeline rows from the rewind truncation ordinal - #72695
Closed
necoweb3 wants to merge 1 commit into
Closed
fix(tui): exclude timeline rows from the rewind truncation ordinal#72695necoweb3 wants to merge 1 commit into
necoweb3 wants to merge 1 commit into
Conversation
prompt.submit resolves truncate_before_user_ordinal against every role=="user" row in session["history"], but no client counts them that way. Bookkeeping timeline rows -- model_switch, async_delegation_complete, auto_continue, hidden -- are stored as durable user rows, and the desktop demotes them to role:'system' (or drops them) in toChatMessages before visibleUserOrdinal counts. The CLI already excludes them with exactly this predicate; the destructive gateway path did not. The ordinal therefore resolved N real turns too early, where N is the number of timeline rows before the target, and the replace_messages() below hard-DELETEd those extra completed exchanges. Neither existing guard fires: the skewed ordinal is always SMALLER, so it stays in range for the 4018 check, and the truncation is non-empty so the 4028 confirm_empty_truncate check does not apply. replace_messages() takes its default active_only=False path, so the rows are gone -- no archive, no mirror, and the desktop has already discarded its optimistic copy. Count the same rows the client counted.
This was referenced Jul 27, 2026
Contributor
|
Thanks for the focused regression report and test case. This is an automated hermes-sweeper review; the requested behavior is already on current
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
prompt.submitresolvestruncate_before_user_ordinalagainst everyrole == "user"row insession["history"]. No client counts them that way. Bookkeeping timeline rows —model_switch,async_delegation_complete,auto_continue,hidden— are stored as durable user rows, but the desktop demotes them torole: 'system'(or drops them) intoChatMessagesbeforevisibleUserOrdinalcounts.The ordinal therefore resolves N real turns too early, and the
replace_messages()that follows hard-DELETEs those extra completed exchanges. Same class as #70895: an index into the frontend's message array treated as an index into the backend's history.Problem
Producer —
apps/desktop/src/app/session/hooks/use-prompt-actions/utils.ts:362:Its only filter is
!m.hidden, a client-side branch flagapplyBranchVisibilitysets on assistant rows. It never needs to know aboutdisplay_kind, because by the time it runstoChatMessageshas already removed those rows from theuserrole —apps/desktop/src/lib/chat-messages.ts:949-955:and
display_kind: 'hidden'yields no parts at all, so the row is skipped entirely.Consumer —
tui_gateway/server.py:10790:No
display_kindexclusion. The marker rows are insession["history"]withrole == "user"—server.py:11828stampsmessage["display_kind"]on the in-memory row after the run, andhermes_state.py:7283rehydrates it on resume.Twin-guard asymmetry. The CLI performs the same logical operation — count user turns in a stored history — and does exclude them, with exactly this predicate:
hermes_cli/cli_agent_setup_mixin.py:518—if m.get("role") == "user" and not m.get("display_kind")hermes_cli/cli_commands_mixin.py:854— same expressionOnly the destructive gateway path omits it.
Neither existing guard fires. The skewed ordinal is always smaller than the true one, so it stays in range for the 4018 bounds check at
server.py:10796— whose own comment already names this hazard class ("silently truncating history to everything before it and persisting that loss viareplace_messages— an unrecoverable overwrite of the session DB"). And the truncation is non-empty, so the 4028confirm_empty_truncateguard (added for #70895/#70516) does not apply either.Reproduced
Running the real client rule and the real gateway expressions over one history:
A completed question and its answer, permanently deleted, with no warning.
Default config.
_AUTO_CONTINUE_ENABLED_DEFAULT = True(server.py:6421);async_delegation_completeis emitted unconditionally by the notification poller (server.py:11379,:11457). No flag or setting is involved on either side.Deterministic. Pure integer arithmetic over two lists — the skew is exactly the number of timeline rows before the target, on the first attempt, every time.
Frequent. Edit-a-message, Regenerate and Restore-checkpoint are always-visible controls and all funnel through this ordinal. Once one timeline row exists it is permanent for the session, so every subsequent rewind in that chat is misaligned, and the skew accumulates with each additional marker.
Unrecoverable.
replace_messagestakes its defaultactive_only=Falsepath —DELETE FROM messages WHERE session_id = ?, whose docstring says "DESTRUCTIVE by default: every row for the session is DELETEd (and drops out of the FTS index)". Nothing holds a copy:archive_and_compact's soft-archive is a different method and is not used here, soinclude_inactive=Truereturns nothing; there is no JSONL mirror on this path; and the desktop already sliced its ownstate.messagesoptimistically, then repaints from the truncated DB on the next resume.Fix
Count the same rows the client counted:
The predicate is copied verbatim from the CLI siblings. It is exactly right for every
display_kindvalue that exists —model_switch,async_delegation_completeandauto_continueare demoted tosystemclient-side, andhiddenis dropped — so no client ever counts any of them.Scope
tui_gateway/server.py— one list comprehension in theprompt.submittruncation branch, plus a comment. No change to the 4018/4028 guards, toreplace_messages, or to any other RPC.Related but distinct: #41275 (open) touches the same
visibleUserOrdinalhelper in the opposite direction — optimistic failed user bubbles make the client ordinal overshoot, producing a visible 4018 rejection. Its fix skips failed turns and cannot correct an undercount, and it describes no silent deletion.Testing
New
test_prompt_submit_ordinal_skips_display_kind_timeline_rowsintests/test_tui_gateway_server.py, built in the same idiom as the existingtest_prompt_submit_can_truncate_before_user_ordinaldirectly above it: a history where a delegation-complete row sits between the first and second real exchanges, then an edit of the third user turn (ordinal 2).It asserts the cut lands at "third" and that "second"/"second reply" survive. Fails on
main— the stored transcript comes back asoriginal_history[:4], having destroyed the second exchange — and passes here.The pre-existing
test_prompt_submit_can_truncate_before_user_ordinalpasses unmodified, so ordinary rewind behaviour is unchanged.Note on
tests/tui_gateway/: that directory has order-dependent failures unrelated to this change — running the full directory on unmodifiedmaingives 6 failures (test_compute_host*,test_projects_rpc*,test_subagent_child_mirror), and 3 on this branch. Each of those tests passes in isolation on both.