Repository navigation
Conversation
ba11b4b to
637d5d4
Compare
|
@OutThisLife friendly ping for a review when you have a moment — this picked up the What it fixes: the desktop "Branch in new chat" context-loss bug. Scope vs #95992: that PR fixed the sibling route/selection ordering issue; this one is complementary — it replaces count-based truncation with durable
Happy to rebase if main has moved, or adjust anything. |
59fe527 to
03e0853
Compare
session.branch truncated the parent's live history with history[:count], where count came from the desktop's toBranchMessages() over the MERGED toChatMessages projection (tool rows folded into assistant bubbles, empty tool-call turns collapsed, timeline markers interleaved). That merged-bubble count has no stable mapping onto the backend's raw row history, so every desktop branch silently sliced off the conversation tail — a 261-row parent forked only its first ~108 rows (NousResearch#80973, partially NousResearch#87949). Two more defects a pure count -> row-id swap would not fix: 1. Merged bubbles span several rows. An auto-continued long answer is persisted as multiple assistant rows plus folded tool rows, but the bubble only carried the FIRST row's rowId. Cutting at that id would still drop the bubble's own tail, so bubbles now carry endRowId — the last durable row of the span they cover — and the cut uses it. 2. Nested branches addressed the wrong database. A branch child starts with its parent's ChatMessage objects (parent row ids) and its live history kept the parent's _row_ids after the copy. A second-generation fork then sent an id that exists in the child's REST transcript but not in its live history. The copy now re-stamps the in-memory history with the child's new SQLite ids (read back with include_row_ids). Backend — session.branch (tui_gateway/methods_session.py) - Whole raw history is copied by default; truncation happens only via up_to_row_id (durable messages.id). - The cut is ORDINAL ("keep rows whose id <= target"), not an exact-member match: a merged bubble's terminal id may belong to a row this projection filtered out (folded tool row, hidden marker), and requiring that exact row would wrongly reject the fork. Ids are monotonically increasing in insertion order, so the ordinal cut lands exactly on the last visible row of the clicked bubble's span. - Id-range guard: a target outside [min, max] of the ids this transcript ever persisted is refused with 4009 branch target row not found — a stale/foreign id can never silently broaden the fork into a full-history branch (the exact failure mode the count-based path had). - After the copy, the in-memory history is re-stamped with the child's new row ids so nested branches resolve against the child's own database. Frontend (desktop) - toChatMessages hydration stamps endRowId on every bubble: continuation rows merged into an active assistant, tool results folded via applyStoredToolResult, and pending tool flushes. rowId keeps naming the first row (reactions still address it). - branchCurrentSession addresses the cut with the terminal bubble's endRowId ?? rowId; a fresh turn that has not round-tripped row ids falls back to resolving against the REST transcript (resolveDurableRowIdForMessage, same-role/text ordinal matching), and if the cut still cannot be addressed the fork is refused with an error instead of silently retargeting. - Whole-chat forks send NO truncation at all; the backend copies everything. - The cut address is part of the branch create-flight identity, so two branches taken at different messages of the same live parent are not coalesced. Contract - session.branch params gain up_to_row_id; the legacy merged-message count is kept in the schema for older senders but IGNORED by the handler. - DESKTOP_BACKEND_CONTRACT / REQUIRED_BACKEND_CONTRACT -> v8 (a v7 backend rejects the new parameter, so the desktop warns to align rather than failing cryptically), with the generated contract artefacts regenerated. Test plan - scripts/run_tests.sh tests/tui_gateway/test_tui_gateway_server.py — 669 passed (covers default full copy, ordinal cut, id-range refusal, nested-branch re-addressing) - vitest use-session-actions + lib/chat-messages + store/updates — 421 passed - tsc --noEmit clean Rebased onto current main (2026-09-20) by @hutao562: ported to the split tui_gateway modules, added the RPC parameter to the gateway contract + bumped the GUI/backend contract to v8, regenerated the contract artefacts, folded the cut address into the create-flight identity, and updated the regression tests.
11efcaa to
042fd47
Compare
|
Rebased onto current Changes vs. the original revision, worth re-reading before review:
Verification on the rebased tree:
|
|
I can confirm that this fixes the issue for me when I patch this into latest main |
|
@hutao562 heads up — when I pulled this onto latest main and rebased, I found that main has since removed In my fork I kept both fields so the PR's branch addressing and main's transcript-tail accounting coexist (the terminal id and the row count are complementary, not duplicates). Worth folding that back into this PR so it merges cleanly against current main — as written it no longer applies without conflicts. Happy to share the reconciliation if useful. |
|
@teknium1 could you take a quick look at #98025 when you get a chance? this is still reproducible on current main, and @tripflex independently confirmed the patch fixes it in real use. the underlying invariant is pretty small: the desktop currently chooses a branch boundary in merged-message space, while the backend truncates raw persisted rows. #98025 makes that boundary a durable row id instead, with guards against stale/foreign ids and coverage for nested branches. we’ve also consolidated the older attempts around this bug so this should be the one review surface. if the protocol shape is the blocker, happy to adjust it — mainly looking for a yes/no on the approach. |
|
did another adversarial pass on this before review. the row-id direction still looks right, but i found a few edge cases worth tightening:
the invariant i'd aim for is: a branch boundary is a durable position in the parent's persisted transcript. if we can't prove that position, refuse the branch rather than approximating it from merged counts, numeric ranges, or partially-addressed live history. also +1 to @tripflex's "serverRowSpan" / "endRowId" reconciliation — those represent different things (cardinality vs durable terminal identity), so keeping both seems cleaner than trying to derive one from the other. i'd add adversarial coverage for: foreign id inside the parent's min/max, unaddressed live tail after the cut, a real folded tool/continuation span, and child → grandchild branching after persistence/reload. |
Summary
Fixes the desktop "Branch in chat" cutting the parent conversation at the wrong place — dropping the conversation tail on whole-chat forks and mis-slicing message-level forks (#80973, partially #87949).
Root cause.
session.branchtruncated the parent's live history withhistory[:count], wherecountcame from the desktop'stoBranchMessages()over the mergedtoChatMessagesprojection (tool rows folded into assistant bubbles, empty tool-call turns collapsed, timeline markers interleaved). The merged-bubble count has no stable mapping onto the backend's raw row history, so the slice landed wherever the two count spaces happened to disagree — a 261-row parent forked only its first ~108 rows.Two more defects a pure
count→ row-id swap would not fix:rowId. Cutting at that id would still drop the bubble's own tail. Bubbles now carryendRowId— the last durable row of the span they cover — and the cut uses it.ChatMessageobjects (parent row ids) and its live history kept the parent's_row_ids after the copy. A second-generation fork then sent an id that exists in the child's REST transcript but not in its live history. The handler now re-addresses the copied history with the child's new SQLite ids (read-back withinclude_row_ids), and the child's initial renderer state prefers the transcript returned bysession.branch.Changes
Backend —
session.branch(tui_gateway/methods_session.py)up_to_row_id(durablemessages.id).[min, max]of the ids this transcript ever persisted is refused with4009 branch target row not found— a stale/foreign id can never silently broaden the fork into a full-history branch (the exact failure mode the count-based path had).countparameter is still honored, so an older desktop against this gateway behaves no worse than today.Frontend (desktop)
toChatMessageshydration stampsendRowIdon every bubble: continuation rows merged into an active assistant, tool results folded viaapplyStoredToolResult, and pending tool flushes.rowIdkeeps naming the first row (reactions still address it).branchCurrentSessionaddresses the cut with the terminal bubble'sendRowId ?? rowId; a fresh turn that has not round-tripped row ids falls back to resolving against the REST transcript (resolveDurableRowIdForMessage, same-role/text ordinal matching), and if the cut still cannot be addressed the fork is refused with an error instead of silently retargeting.Test plan
tsc --noEmitcleanvitest:use-session-actionssuite — 246 passed (includes mainline's new connection-routing tests + this PR's merged-count/row-id/refusal tests)tests/test_tui_gateway_server.pycovers the ordinal cut, id-range guard, and nested-branch re-addressing