Skip to content

fix(desktop): branch cuts the parent transcript at a durable row id - #128550

Open
OutThisLife wants to merge 3 commits into
mainfrom
bb/80973-branch-row-id
Open

OutThisLife wants to merge 3 commits into
mainfrom
bb/80973-branch-row-id

Conversation

@OutThisLife

Copy link
Copy Markdown

fix(desktop): branch cuts the parent transcript at a durable row id

Root cause

The desktop sent a merged-bubble count and the backend sliced raw rows with
history[:count]; the two count spaces have no stable mapping (tool rows fold
into assistant bubbles, compaction replaces the live projection), so the cut
landed early and silently dropped the conversation tail. A second-generation
branch failed outright ("row not found") because the child kept the parent's
row ids.

What changes

Consolidates the two contributor approaches, contract-first:

  • Contract (@hutao562): session.branch params gain up_to_row_id
    (OpenRPC + regenerated TS); the legacy count is documented as IGNORED.
    The desktop resolves the clicked bubble's terminal durable row id
    (resolveDurableRowIdForMessage, with a REST-transcript fallback) and
    refuses the branch when the cut target cannot be resolved — no live-atom
    guess, no silent early cut.
  • Backend (@kingOfSoySauce, adapted): session.branch truncates by
    up_to_row_id and errors 4009 on an unknown row; the copied child rows are
    re-addressed with their NEW child-db row ids (append_messages_batch
    return ids re-stamped into the live history), so a second-generation branch
    can address its cut.
  • Whole-chat branches stay on main's server-side display projection
    (session.branch_whole), untouched.

Acceptance coverage

#80973: whole-chat branch copies the parent's complete raw history (count
mismatch ignored); fork-from-message cuts at the clicked bubble's exact raw
row; child keeps valid row ids (second-generation branch works).

#87949: message-level branch of a compacted long session copies the full
persisted history up to the cut — new regression test
test_session_branch_compacted_parent_cuts_on_persisted_rows (614-row parent
compacted to summary+tail, branched at a mid-archived-region bubble, asserts
the child sees the persisted rows).

Tests (all real runs)

  • HERMES_PYTHON=… bash scripts/run_tests.sh -j 2 --file-timeout 1500 tests/tui_gateway/test_tui_gateway_server.py -k "branch" — 13/13 passed at
    head 6f0da767e7. Red on origin/main (40e4af42b8e, test file copied
    in): 2 failed (test_session_branch_full_history_by_default_truncates_by_row_id,
    test_session_branch_compacted_parent_cuts_on_persisted_rows).
  • cd apps/desktop && npx vitest run --maxWorkers=2 src/app/session/hooks/use-session-actions.test.tsx src/app/session/hooks/use-session-actions/utils.test.ts — 266/266 passed
    at head. Red on origin/main: 3 failed (resolves the matching occurrence when live messages have ephemeral ids, returns undefined when the selected message is not in the persisted transcript, branches from a specific message by durable row id).

Note: #80970's orphaned-tool-call pairing guard was evaluated and dropped —
main's display-projection branch source (_visible_branch_history) filters
tool rows out of branch copies entirely, so the guard can never fire on this
path. Its Electron packaging files were never carried. #80970's tile-fork
wiring is superseded separately by #128549 (the focused tile-fork PR), per the
routing verdicts.

Fixes #80973
Fixes #87949

Supersedes #98025
Supersedes #80970

Thanks @hutao562 — the contract-first commit (38ac027) is theirs and is the
base of this branch. Thanks @kingOfSoySauce — the backend row-id restamping
and 4009 guard are ported from #80970.

Co-authored-by: Leon 1172300059@qq.com

@github-actions

github-actions Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

૮ >ﻌ< ა ci review

ran on c263fba — test(desktop): drop the duplicate routedSessionId in the war

debug info

CI timings

CI timings · View report · View job

Wall time 15m13s vs 33m52s (-55.1%). 4 job(s) slower, 12 faster,

  • OS-specific tests / Windows-only tests: -641.0s
  • OS-specific tests / Windows-only tests (arm64): +179.0s
  • Check no case-colliding filenames / check-case-collisions: -144.0s
  • OS-specific tests / macOS-only tests: -140.0s
  • No production imports of the tools.lazy_deps stub / No production imports of the tools.lazy_deps stub: -46.0s

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Sep 29, 2026
@tripflex

Copy link
Copy Markdown

Can we pleasssseee get this fixed, branching is broken and i don't understand why this isn't a higher priority, i seriously can't be the only one who is constantly having this issue

@tripflex

Copy link
Copy Markdown

@teknium1 @kshitijk4poor please review <3 thank you <3

@OutThisLife
OutThisLife force-pushed the bb/80973-branch-row-id branch 4 times, most recently from 25b4419 to 3f35635 Compare October 1, 2026 07:35
kingOfSoySauce and others added 3 commits October 1, 2026 03:04
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 (#80973, partially #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.
@OutThisLife
OutThisLife force-pushed the bb/80973-branch-row-id branch from 3f35635 to c263fba Compare October 1, 2026 08:04

kvnloo commented Oct 1, 2026

Copy link
Copy Markdown

Branch-contract review: moving the cut from merged-message count to durable up_to_row_id is the right boundary. It makes the backend truncate raw history in its own coordinate space, and re-stamping copied child rows with their new DB ids preserves second-generation branches. Refusing an unresolved message cut is also preferable to silently guessing. I don't see a correctness blocker in the current contract; this should be the base that tile/message branch callers layer onto.

@mzyas

mzyas commented Oct 2, 2026

Copy link
Copy Markdown

Data point for a variant of #80973 that this PR should cover: a transcript containing a notice row (e.g. `model_switch`, persisted as `role=user` + `display_kind`) also breaks the count cut. The renderer turns it into a `role: 'system'` / `systemNotice` message that `toBranchMessages` filters out, while `_visible_branch_history` keeps it, so `history[:count]` lands one row early and "Branch in new chat" on the last reply drops it. Measured on a real session: parent 17 visible rows, child 16.

From reading the diff, the ordinal `_row_id <= up_to_row_id` cut keeps notice rows regardless of how the renderer counts, so this should be fixed here. I haven't run the branch. Might be worth a regression case with a notice row before the clicked message.

@alt-glitch alt-glitch added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Oct 2, 2026
@GrgaPalcic

Copy link
Copy Markdown

I rebased this PR on main at 97a9ae8ee5. The branch has no conflicts:
https://github.com/GrgaPalcic/hermes-agent/commits/fix/desktop-branch-row-id

If it helps, you can take these commits into this PR.

The first two commits come from this PR, and their authors do not change. The third PR commit is not necessary now (see below). The message of 9ba33b1571 lists how I resolved each conflict:

Fixes

  • 420a34ba44: the backend contract is now v9. feat(connectors): the backend API for the desktop Connectors page; connect an app without a chat session #115191 used v8 for the Connectors RPCs. A v8 backend from main passes the desktop check, but it rejects up_to_row_id with error 4000. Thus "branch from a message" fails, and the desktop shows no align prompt.
  • 34a6b0bfb3: the REST fallback (resolveDurableRowIdForMessage) returned rowId. That is the first row of a bubble. An answer with a tool call has three or more rows. Thus the branch lost the end of that answer. The fallback now returns endRowId. The new test fails before this commit and passes after it.
  • 64e9d29c83: _branch_live did the child row-id re-stamp a second time, after _persist_branch. This commit removes the second copy.

Code health. The ratchet became blocking on 2026-10-05, after the last CI run of this PR. The PR made 8 units larger than their caps. These commits move code and tests. They do not change behavior, and they add no # health: allow:

  • a8156e358f and daef1760f0 move the new tests into new files.
  • 9fdf51450c moves _persist_branch into methods_session_branch.py.
  • a68e889c07 moves the cut helpers into use-session-actions/branch-cut.ts.
  • 6c15c2b24f moves two endRowId guards out of toChatMessages.

tui_gateway/server.py grows by 3 comment lines (the v8 and v9 notes). Its existing file-level # health: allow FILE_LINES covers this.

Tests

  • python scripts/check: pass.
  • scripts/run_tests.sh tests/tui_gateway/: 2527 passed, 0 failed.
  • Desktop: npm run typecheck passes. The hook, chat-message and update tests pass.
  • Each commit passes the branch tests alone.

Real session (Linux desktop build)
The parent session has 120 rows and 7 user turns. I made a branch from the same message two times:

  • Before (a desktop build without this fix): 12 rows, 4 user turns. The cut was at row 53 of the parent.
  • After (the fix of this PR, before the follow-up commits): 21 rows, 6 user turns. The cut is at row 90 of the parent. That is the last row of the selected answer, which has 6 text rows.

Related

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

7 participants