Keep state.db tool-call rows before final answers

[franksong2702]

Thinking Path

• /api/session merges WebUI JSON sidecar messages with Hermes Agent state.db messages before returning a paginated transcript window.
• Recent long sessions could have a complete final answer in the sidecar while state.db still contributed older empty-content assistant rows carrying distinct tool_calls.
• The existing merge path correctly preserved those distinct tool-call rows, but preserved them by falling through to the generic append path.
• That could move older tool-call-only assistant rows after the settled final answer on paginated loads, making the renderer treat the real final answer as a non-final assistant segment until a later refresh/full load.
• The fix keeps the state layer behavior narrow: preserve the tool-call rows, but insert them chronologically instead of appending them to the tail.

What Changed

• Updated merge_session_messages_append_only() so state.db assistant rows with distinct tool_calls inside the sidecar timestamp range are inserted by timestamp.
• Kept the existing dedupe semantics for duplicate tool calls and non-tool assistant rows.
• Added a regression test that reproduces an older state.db tool-call-only assistant row arriving after a sidecar final answer and asserts the final answer remains the merged transcript tail.
• Removed the contributor CHANGELOG.md entry from this PR to avoid release-note conflicts.

Why It Matters

Paginated session loads should not make a complete final answer look incomplete just because an older process/tool-call row was reconciled from state.db. This keeps WebUI/state.db reconciliation from breaking the final visible reply boundary while still preserving distinct tool activity rows.

This is related to the WebUI sidecar / Hermes state.db reconciliation work in #4834, but it is a separate ordering bug in the shared append-only merge path. It also follows the tool-call preservation behavior introduced around #3665 and the later edit/retry/undo reconciliation work for #4767/#4772: those rows should be preserved, but not moved after settled final answers.

Verification

• python3 -m py_compile api/models.py
• git diff --check origin/master...HEAD
• ./scripts/test.sh tests/test_merge_key_tool_calls.py tests/test_webui_state_db_reconciliation.py tests/test_core_data_loss_cases.py tests/test_session_message_window_renderable_tail.py -q — 62 passed.

Risks / Follow-ups

• This changes only the merge ordering for preserved distinct state.db tool-call assistant rows; it does not drop any tool calls.
• The touched state layer is the WebUI JSON sidecar + Hermes Agent state.db transcript merge used by /api/session paginated loads.
• This does not change live streaming, PWA behavior, Assistant Turn Anchor frontend logic, or Desktop live mirroring.

Model Used

OpenAI Codex GPT-5 with local repository inspection, regression tests, and GitHub CLI publishing.

[greptile-apps]

Greptile Summary

This PR fixes a transcript ordering bug in the WebUI sidecar / Hermes state.db reconciliation path: when state.db contributes tool-call-only assistant rows already within the sidecar timestamp range, the old pass-then-append path always moved them to the transcript tail, making a settled final answer look non-final to the renderer.

• _tool_call_assistant_should_precede_content_assistant is added and wired into both the should_insert condition and the advance-loop guard inside _insert_state_message_chronologically, ensuring equal-timestamp tool-call rows are placed before content-only final answers.
• Pre-window rows (timestamp strictly before the earliest sidecar message) are intentionally dropped — the new test test_pre_window_state_tool_call_row_is_not_tail_appended validates this as correct behavior for no-watermark resurrection candidates.

Confidence Score: 4/5

The change is safe for the common case; the new chronological-insert path is well-tested. One narrow ordering gap remains when a final answer simultaneously carries content and tool_calls at the same timestamp as the state.db row being reconciled.

The core fix works correctly for documented scenarios and is backed by three targeted regression tests. The guard's not existing.get('tool_calls') condition leaves an equal-timestamp gap when the final answer also has tool_calls, which is uncommon but reachable with agentic models.

api/models.py — the new _tool_call_assistant_should_precede_content_assistant helper and its integration in the advance loop.

Important Files Changed

Filename	Overview
api/models.py	Adds _tool_call_assistant_should_precede_content_assistant helper and plumbs it into _insert_state_message_chronologically for tie-breaking at equal timestamps; replaces the pass/tail-append path with a chronological insert, intentionally dropping pre-window resurrection candidates when _insert_state_message_chronologically returns False. The guard works for content-only final answers but leaves a narrow gap when a final answer carries both content and tool_calls at the same timestamp as the state.db row.
tests/test_merge_key_tool_calls.py	Adds three regression tests covering older-timestamp, equal-timestamp, and pre-window cases; all accurately target the ordering invariant that the final answer must remain the transcript tail. Test helper and assertions are well-structured and clearly document the intentional drop behavior for pre-window rows.

_{Reviews (2): Last reviewed commit: "Handle equal-timestamp tool-call tail or..." | Re-trigger Greptile}

[nesquena-hermes]

Read the changed branch in merge_session_messages_append_only against origin/master:api/models.py (the pass # different tool_calls from sidecar — preserve fallthrough is still live on master at ~5642), plus the full body of _insert_state_message_chronologically (models.py:5233). The fix is correct and the change is well-scoped.

The bug this fixes is real

On master, a state.db assistant row with distinct tool_calls inside the sidecar timestamp range hit:

if _ck in seen_content_keys and dedup_key not in seen_dedup_keys:
    pass  # different tool_calls from sidecar — preserve

That pass fell through to the generic tail-append, so an older tool-call-only row reconciled from state.db landed after a settled final answer already present in the sidecar. The renderer keys "is this the final reply" off the transcript tail, so the complete answer got demoted to a non-final assistant segment until a full reload re-sorted things. The PR replaces the fallthrough with _insert_state_message_chronologically(...) + continue, which inserts at the timestamp slot and keeps the final answer at the tail. The new test_older_state_tool_call_assistant_stays_before_final_answer reproduces exactly this (sidecar tool t=1000.0, final t=1001.0, state tool t=1000.5) and asserts result[-1] == final_answer.

On greptile's "silently dropped when _insert... returns False" concern

I looked at this specifically and I don't think it's a regression — the new behavior is actually safer than the old one. _insert_state_message_chronologically only returns False in one shape (models.py:5258):

if idx == 0 and existing_timestamp is not None and existing_timestamp > timestamp:
    # With no surviving sidecar/context row before this slot, a real
    # interruption rescue is indistinguishable from a compacted-out old
    # prompt; prefer avoiding no-watermark resurrection in that shape.
    return False

That is: the row predates every row already merged. In that case the OLD code's tail-append was doubly wrong — it would have put a row that's chronologically the oldest at the very end of the transcript, after the final answer. So the previous behavior didn't preserve order, it actively corrupted it for precisely the case this PR set out to fix. Dropping a pre-window resurrection candidate is the intended no-watermark guard (same rationale the function already applies to the user and other state-only branches that call it). The other two callers in this same function (the watermark fall-through user branch just below, line ~5658) already rely on this exact if _insert...(): ... / else-skip contract, so 4893 just makes the tool-call branch consistent with them rather than inventing new semantics.

Suggestion (optional, not blocking)

If you want to close greptile's documentation gap, add one more assertion to the test for the False shape — a state tool-call row with a timestamp before all sidecar rows — asserting the final answer still ends up last and the transcript isn't corrupted. That converts the "undocumented edge" into a guarded one. Not required for correctness, but it'd preempt the exact review question.

CI is green across the full matrix, the diff touches only the one merge branch (no change to dedupe of duplicate tool calls or non-tool rows), and removing the contributor CHANGELOG entry was the right call given the release-bot convention. Looks merge-ready.

[nesquena-hermes]

Nice fix — the core is correct and a strict improvement over master: routing the distinct-tool-call row through _insert_state_message_chronologically keeps older tool-call-only rows before the settled final answer on the common (strictly-older timestamp) case, and the full suite is green. Both gates (Codex + Opus, independently) confirmed Q1 (the False-return drop is only the documented no-watermark-resurrection guard, matching the shipped user-row pattern — safe), Q2 (no tool-call→result block split), and Q3 (no watermark/#4772 regression).

One CORE edge to finish before merge — the equal-timestamp tie (both gates reproduced it independently):

When the state.db tool-call row's timestamp exactly ties the final answer's (realistic under second-granularity stamps — a fast tool call + the final answer in the same integer second; note your test helpers default to integer timestamp=1000), the headline mis-ordering still happens:

order: ['call_1', 'Final answer.', 'call_2']   # final answer is NOT last

Root cause is two-part, and the second part is the trap:

1. should_insert (api/models.py:~5319) only has an equal-timestamp insert rule for msg.role=="user". A tool-call row is role=="assistant", so at a tie it never triggers and the helper appends at the tail.
2. Even if you add an assistant+tool_calls-before-assistant+content disjunct to should_insert, the fixpoint guard (b) at ~5405 defeats it — guard (b) advances the insertion point past any equal-timestamp run whose left neighbour shares msg's role, so it skips right back past the final answer (I verified this: adding only the should_insert disjunct still produced [call_1, Final answer, call_2]).

So the complete fix needs BOTH: (a) the equal-timestamp should_insert disjunct for an empty-content tool_calls assistant before a content-bearing assistant, AND (b) an exception in guard (b) so it does NOT advance past the content-bearing assistant you're trying to insert before — scoped carefully so the existing user-role path and the tool-pair (a) protection are untouched.

I held off applying this myself because guard (b) is shared with the user-role insert path on the crown-jewel append-only merge, and getting the scoping wrong risks reordering user turns — your call on the cleanest shape is better here. Please also add two tests to tests/test_merge_key_tool_calls.py: (1) the exact-tie case asserting result[-1] is the final answer + both tool-call rows preserved; (2) the False-return drop case (pre-window empty-content tool row) locking in the documented drop. Once that's in, I'll re-run the full Codex+Opus+suite gate and move it through.

[franksong2702]

Addressed the equal-timestamp review blocker on current origin/master.

What changed:

• Merged the branch up to current master.
• Added a narrow equal-timestamp ordering rule so empty-content assistant tool_calls rows insert before a content-bearing assistant final answer.
• Scoped the same-role fixpoint guard so it does not advance past that final-answer slot, while leaving the existing user-row and assistant-tool-result block protections intact.
• Added the two requested regressions in tests/test_merge_key_tool_calls.py: exact timestamp tie keeps the final answer at the tail, and pre-window empty tool-call rows remain dropped by the no-watermark guard.

Verification:

• python3 -m py_compile api/models.py
• python3 scripts/ruff_lint.py
• git diff --check
• ./scripts/test.sh tests/test_merge_key_tool_calls.py -q — 15 passed
• ./scripts/test.sh tests/test_webui_state_db_reconciliation.py tests/test_core_data_loss_cases.py tests/test_session_message_window_renderable_tail.py tests/test_merge_key_tool_calls.py -q — 67 passed
• GitHub CI is green on 11c263ec6.

Ready for re-review.

[nesquena-hermes]

Shipped in v0.51.670 (Release XZ, just deployed) — thanks @franksong2702! The equal-timestamp tie edge is resolved: settled tool-call rows now stay before the final answer even when a fast tool call and the final answer land in the same second. Gate: Codex SAFE + Opus SHIP (user-path provably byte-identical — the guard-b exception is scoped to the tool-call-before-content-assistant case and can't fire for user rows; tool-result block integrity + watermark reconciliation unchanged); I added the two coverage tests Opus flagged (multi-tool tie + block-no-split). Suite 10643. Verified on prod.