Edit/regenerate/fork derive keep_count from DOM index, not DB position → wrong truncation/fork point (pre-existing; make id-based)

[jdmanring]

Summary

Edit, regenerate, and fork compute the server-side keep_count from a message's position among the currently-rendered .msg DOM elements, but the server applies keep_count as an absolute index into the full, timestamp-ordered DB message list. Those two numbers are equal only under assumptions that are routinely false, so these operations truncate or fork the conversation at the wrong message — silently deleting messages the user meant to keep, or forking the wrong slice.

Pre-existing in both fork and upstream (not introduced by the DOM-virtualization work, #2). Filed as its own upstream-candidate item.

The violated invariant (verified against code)

Client — keep_count = indexOf('.msg'):

• editUserMessage (static/js/chat.js): allMsgs = box.querySelectorAll('.msg'), msgIndex = allMsgs.indexOf(userMsgElement), then keepCount = msgIndex → POST /api/session/{id}/truncate {keep_count} (~chat.js:4468, 4512).
• regenerateFrom: same shape, keepCount = userIndex → /truncate (~chat.js:4556, 4603, 4717).
• forkFrom: aiIndex = allMsgs.indexOf(aiMsgElement), keepCount = aiIndex + 1 → POST /api/session/{id}/fork {keep_count} (~chat.js:4885, 4891).

Server — keep_count is an absolute DB index:

• POST /truncate (routes/history/history_routes.py:252) → SessionManager.truncate_messages(session_id, keep_count) (core/session_manager.py:290): queries all rows order_by(timestamp), deletes db_messages[keep_count:], and sets session.history = session.history[:keep_count].
• POST /fork (routes/history/history_routes.py:597): msgs_to_copy = source.history[:keep_count] (line 623) — absolute slice of the full history.

indexOf('.msg') equals the absolute DB index only if every DB message renders exactly one .msg element and the DOM begins at DB index 0. Neither holds in general.

Three concrete divergence vectors (each verified)

1. Windowing / pagination — DOM does not start at DB index 0. The chat log is virtualized (MessageWindow, static/js/chatHistory.js) and renders a bounded window with a serverOffset into the DB; upstream likewise loads only a tail page (24 desktop / 8 mobile) with the server defaulting page_offset = max(total - page_limit, 0). So the first rendered .msg sits at DB index serverOffset (or page_offset), and indexOf understates the absolute index by that offset. keep_count too small → truncate/fork cuts earlier than intended and deletes messages the user wanted to keep.

2. Multi-bubble agent replies — one DB row → many .msg. chatRenderer.addMessage renders a multi-round assistant message as several bubbles: wrap.className = 'msg msg-ai' + (r > 0 ? ' msg-continuation' : '') (chatRenderer.js:2287). So a single DB message produces N .msg elements, and indexOf overstates the absolute index.

3. Synthetic turns — DB rows with no .msg. _mapHistoryMessages (static/js/sessions.js:112-131) deliberately drops the synthetic "Continue where you left off" / instruction user turns from rendering. Those are real DB rows that produce no .msg, so indexOf understates the absolute index.

Because these push the index in both directions and depend on scroll position, conversation shape, and agent activity, there is no constant offset that corrects indexOf — the mapping is genuinely wrong, not merely shifted.

Failure scenario

Open a conversation long enough to be windowed/paginated (so the first rendered .msg is at DB offset > 0), or one containing a multi-round agent reply, then click edit on an earlier message or fork from an assistant reply. The server slices at the DOM-derived index, not the intended message: truncate deletes the wrong tail (data loss), fork copies the wrong prefix. On a short, single-page, chat-only conversation the numbers happen to line up, which is why this passes casual testing.

Relationship to #2 (not a blocker)

This defect predates and is independent of the message-window rewrite (#2). #2 actually reduces the fresh-load blast radius — it renders the whole conversation up to 50 messages on load (vs upstream's 24), so the numbers line up for more conversations. The one narrow regression #2 introduces is that eviction removes upstream's "scroll to the very top, load everything, and then the indices line up" recovery path for plain conversations above the co-resident cap. Net, #2 is an improvement here. Fixing this properly cures both branches and makes #2's eviction irrelevant to correctness.

Proposed fix — make truncate/fork id-based (the infrastructure already exists)

Delete already works this way, end to end — reuse the same machinery:

• .msg elements already carry their DB id: wrap.dataset.dbId = metadata._db_id (chatRenderer.js:2323, 2498).
• deleteMessages (chat.js:5213) reads dataset.dbId and sends { msg_ids }.
• The server /delete-messages endpoint (routes/history/history_routes.py:284) resolves ids via a _get_db_id(m) helper and filters session.history = [m for m in session.history if _get_db_id(m) not in msg_ids].

Server: add truncate_after_msg_id(session_id, msg_id) (delete every DB row after the one with msg_id; keep through it) and fork_after_msg_id(session_id, msg_id) (copy history up to and including msg_id), both resolving position with the existing _get_db_id against the timestamp-ordered rows.

Client: editUserMessage/regenerateFrom send the target user message's dataset.dbId (truncate to just before it); forkFrom sends the AI message's dataset.dbId (fork through it). Drop the indexOf/keep_count computation.

Boundary semantics to get right per operation:

• Continuations share the parent message's _db_id, so a multi-bubble reply collapses to one boundary id automatically — the whole reason to switch off DOM counting.
• Messages without a _db_id (unpersisted/error outputs, cf. #1428) must fall back gracefully exactly as deleteMessages already does (DOM-only removal, no server call).

Test plan

• Server unit tests for truncate_after_msg_id / fork_after_msg_id on a seeded session that includes a synthetic "Continue…" turn and a multi-round assistant message, asserting the exact surviving/copied DB rows.
• A windowed/paginated scenario (DB offset > 0) asserting the correct rows remain after an edit/fork — the case the current index-based code gets wrong.

Classification

Upstream-candidate (server + client), pre-existing in both branches. Own branch/PR when scheduled; not a #2 blocker.

[github-actions]

⚠️ Issue description is incomplete. Please update the following sections:

• Install Method — select how you installed Odysseus
• Operating System — select your OS
• Steps to Reproduce — must include at least one numbered or bulleted step
• Expected Behaviour — section is empty or too short
• Actual Behaviour — section is empty or too short

This comment is deleted automatically once all sections are complete.

[jdmanring]

Implemented on fix/truncate-fork-by-msg-id (single clean commit c3ec7cf7 off upstream-mirror), cherry-picked to develop (d0b145e0), both pushed.

• Precondition _db_history_entry now stamps _db_id into the paginated /history payload — this also repairs deleteMessages on scrolled-back/windowed history, which was silently DOM-only before (loaded .msg had no dataset.dbId). That's why the /history change rides along with this truncate/fork fix.
• SessionManager.truncate_from_message + /truncate from_msg_id + /fork through_msg_id resolve the cut point by DB id independently within each store (no shared positional index); client sends dataset.dbId; keep_count retained as fallback (AI-skills tool + unpersisted messages).
• Tests tests/test_truncate_fork_by_msg_id.py: truncate/fork-by-id over a history with a synthetic "Continue…" turn + multi-round reply (the discriminating case), unknown-id no-op, the _db_id precondition via the real paginated endpoint, and a client guard. Fail-red verified on the precondition.
• No-regression: full -k (history or truncate or fork or session) suite green (436 passed) — confirms the /history shape change breaks no existing consumer.

Stays open pending the upstream PR (upstream-candidate). Depends on #125 for the paginated endpoint to be live.