From 7a30ea5f39ea471e009bede227c6a217c9056218 Mon Sep 17 00:00:00 2001 From: Anthony Ruiz Date: Tue, 25 Aug 2026 00:01:35 +0000 Subject: [PATCH] fix(api): fence chronological state merge by provenance --- ARCHITECTURE.md | 17 +- api/models.py | 21 +- api/routes.py | 1 + ..._issue_state_row_id_chronological_order.py | 211 +++++++++++++----- 4 files changed, 173 insertions(+), 77 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d4ca8243..2ce017cf 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -241,12 +241,17 @@ Called after run_conversation() completes to set the session title retroactively `reconciled_state_db_messages_for_session()` uses `merge_session_messages_append_only()` to combine a WebUI sidecar or context -projection with active Agent `state.db` rows. “Append-only” is a preservation -contract, not an append-at-the-tail ordering rule: a `state.db`-only row whose -timestamp predates the sidecar tail is inserted into a safe chronological slot. -This includes typed database row IDs that are absent from the sidecar because -the stores do not share an ID space. Paginated `msg_limit` consumers rely on -this merged order directly rather than applying a later timestamp sort. +projection with active Agent `state.db` rows. Its explicit +`incoming_provenance="state_db"` fence permits a state-only row whose timestamp +predates the sidecar tail to use a safe chronological slot. Paginated +`msg_limit` consumers rely on this merged order directly rather than applying a +later timestamp sort. + +The same merge helper also stitches ordered child sidecars onto archived +compression parents. Those calls leave incoming provenance unverified, so their +stable message sequence remains append-only even when parent rows carry later, +restamped timestamps. Timestamp alone is never authority to move a continuation +inside its parent transcript. If an older row could only be placed before the first surviving sidecar/context row, the insertion helper declines it to avoid resurrecting compacted history. diff --git a/api/models.py b/api/models.py index 49ecb580..feed1aea 100644 --- a/api/models.py +++ b/api/models.py @@ -10420,6 +10420,7 @@ def merge_session_messages_append_only( *, truncation_watermark=None, truncation_boundary=None, + incoming_provenance: Literal["unverified", "state_db"] = "unverified", ) -> list: """Merge sidecar/context and state.db messages without deleting local rows. @@ -11038,19 +11039,14 @@ def _merge_session_messages_append_only_impl( seen_dedup_keys.add(dedup_key) seen_content_keys.add(content_key) seen_visible_keys.add(visible_key) - # Append-only must not mean append-at-the-end. A state.db-only row can - # legitimately reach this point while carrying a timestamp that predates - # the sidecar tail — most often when its typed row id is unique to - # state.db (row_id_preserves_source_multiplicity), which bypasses the - # sidecar-timestamp-range block above and its chronological insert. - # Blindly appending such a row renders it after turns that already - # answered it: the reported shape is a user prompt displayed below its - # own assistant reply on the paginated (msg_limit) load, which unlike - # the full display path does not re-sort by timestamp afterwards. - # Place the row in its chronological slot instead; rows that are - # genuinely newest still land at the tail, so no row is ever dropped. + # This terminal path is shared by state.db reconciliation and by ordered + # sidecar stitching (notably compression continuations). Only the caller + # that read state.db may authorize timestamp-based recovery placement; + # stable child-sidecar sequence remains authoritative even when an + # archived parent was restamped later. if ( - max_sidecar_timestamp is not None + incoming_provenance == "state_db" + and max_sidecar_timestamp is not None and timestamp is not None and timestamp < max_sidecar_timestamp ): @@ -11177,6 +11173,7 @@ def reconciled_state_db_messages_for_session( state_messages, truncation_watermark=getattr(session, "truncation_watermark", None), truncation_boundary=getattr(session, "truncation_boundary", None), + incoming_provenance="state_db", ) return _state_db_session_messages_result( reconciled_messages, diff --git a/api/routes.py b/api/routes.py index daca7c80..86d75dfa 100644 --- a/api/routes.py +++ b/api/routes.py @@ -9296,6 +9296,7 @@ def _limited_webui_messages_for_display_with_sidecar( state_db_messages, truncation_watermark=getattr(session, "truncation_watermark", None), truncation_boundary=getattr(session, "truncation_boundary", None), + incoming_provenance="state_db", ) if cache_key is not None: _state_key = cache_key[4] diff --git a/tests/test_issue_state_row_id_chronological_order.py b/tests/test_issue_state_row_id_chronological_order.py index b3d5fb66..a53c27c3 100644 --- a/tests/test_issue_state_row_id_chronological_order.py +++ b/tests/test_issue_state_row_id_chronological_order.py @@ -1,23 +1,10 @@ -"""Regression: a state.db-only row must never render after a newer reply. +"""Regression coverage for source-aware transcript merge ordering.""" -Real-world shape (session 20260822_084749_7d1e2c, reported 2026-08-23): +import sqlite3 +from types import SimpleNamespace -A user prompt exists in state.db with a UNIQUE typed row id that the sidecar -does not carry (the two stores do not share an id space). That makes -``row_id_preserves_source_multiplicity`` true, which bypasses the -sidecar-timestamp-range block in ``merge_session_messages_append_only`` -- -including the ``_insert_state_message_chronologically`` call that block owns -- -and drops the row onto the terminal ``merged_messages.append``. - -The row therefore renders LAST, underneath the assistant turn that answered it. -The full display path hides this because it re-sorts by timestamp afterwards; -the paginated path (``msg_limit``, used for the initial render) does not, so the -user sees their own message below its own answer. - -The merge itself must produce a chronologically coherent transcript. -""" - -from api.models import merge_session_messages_append_only +import api.models as models +import api.routes as routes def _user(content, ts, **extra): @@ -28,54 +15,160 @@ def _assistant(content, ts, **extra): return {"role": "assistant", "content": content, "timestamp": ts, **extra} -def test_unique_state_row_id_user_row_keeps_chronological_slot(): - """A state-only user row must land before the reply it triggered.""" - # Sidecar: the settled transcript. Its rows carry ids from the sidecar's - # own numbering space. +def _read_idless_state_rows(monkeypatch, tmp_path, rows): + """Project the production shape from a schema with no durable row id.""" + db_path = tmp_path / "state.db" + with sqlite3.connect(db_path) as conn: + conn.execute( + "CREATE TABLE messages " + "(session_id TEXT, role TEXT, content TEXT, timestamp REAL)" + ) + conn.executemany( + "INSERT INTO messages VALUES (?, ?, ?, ?)", + [ + ("ordering-session", row["role"], row["content"], row["timestamp"]) + for row in rows + ], + ) + monkeypatch.setattr(models, "_active_state_db_path", lambda: db_path) + return models.get_state_db_session_messages("ordering-session") + + +def test_idless_state_db_projection_keeps_older_row_in_chronological_slot( + monkeypatch, tmp_path +): + """The SQLite reader must not turn its row identity into a WebUI stable id.""" sidecar = [ - _user("question un", 1000.0, id=1), - _assistant("reponse un", 1010.0, id=2), - _assistant("conclusion finale", 1200.0, id=3), + _user("question one", 1000.0, id="parent-user"), + _assistant("answer one", 1010.0, id="parent-answer"), + _assistant("final conclusion", 1200.0, id="parent-final"), + ] + state = _read_idless_state_rows( + monkeypatch, + tmp_path, + [_user("question two", 1100.0)], + ) + + assert state == [_user("question two", 1100.0)] + + session = SimpleNamespace( + session_id="ordering-session", + messages=sidecar, + truncation_watermark=None, + truncation_boundary=None, + ) + merged = models.reconciled_state_db_messages_for_session( + session, + state_messages=state, + ) + + assert [message["content"] for message in merged] == [ + "question one", + "answer one", + "question two", + "final conclusion", ] - # state.db: the same conversation, but this prompt was persisted only here, - # under a typed row id that is unique to state.db and absent from the - # sidecar. Its timestamp sits BEFORE the settled conclusion above. - state = [ - _user("question deux", 1100.0, id=848467), - ] - merged = merge_session_messages_append_only(sidecar, state) - - contents = [m.get("content") for m in merged] - assert "question deux" in contents, ( - "the state-only user row must be preserved (append-only contract)" - ) - - # The core invariant: the recovered prompt must not sit after the newer - # assistant turn. - assert contents.index("question deux") < contents.index("conclusion finale"), ( - "state-only user row rendered AFTER a newer assistant reply: " - f"{contents}" - ) - - # And the transcript as a whole must be non-decreasing in time. - timestamps = [m.get("timestamp") for m in merged] - assert timestamps == sorted(timestamps), ( - f"merged transcript is not chronologically ordered: {timestamps}" - ) - - -def test_unique_state_row_id_row_still_appends_when_newest(): - """A genuinely newer state-only row still belongs at the tail.""" +def test_idless_state_db_projection_still_appends_genuinely_newest_row( + monkeypatch, tmp_path +): sidecar = [ - _user("question un", 1000.0, id=1), - _assistant("reponse un", 1010.0, id=2), + _user("question one", 1000.0, id="parent-user"), + _assistant("answer one", 1010.0, id="parent-answer"), + ] + state = _read_idless_state_rows( + monkeypatch, + tmp_path, + [_user("late question", 2000.0)], + ) + + session = SimpleNamespace( + session_id="ordering-session", + messages=sidecar, + truncation_watermark=None, + truncation_boundary=None, + ) + merged = models.reconciled_state_db_messages_for_session( + session, + state_messages=state, + ) + + assert [message["content"] for message in merged][-1] == "late question" + + +def test_private_state_db_provenance_can_reorder_terminal_conflict_row(): + """Only a private state.db row identity can authorize terminal reordering.""" + sidecar = [ + _user("question one", 1000.0, id="sidecar-user"), + _user( + "recovered question", + 1100.0, + _state_db_row_id=848467, + api_content="wire-sidecar", + ), + _assistant("final conclusion", 1200.0, id="sidecar-final"), ] state = [ - _user("question tardive", 2000.0, id=848468), + _user( + "recovered question", + 1100.0, + _state_db_row_id=848467, + api_content="wire-state-db-conflict", + ) ] - merged = merge_session_messages_append_only(sidecar, state) + merged = models.merge_session_messages_append_only( + sidecar, + state, + incoming_provenance="state_db", + ) - assert [m.get("content") for m in merged][-1] == "question tardive" + assert [message["content"] for message in merged] == [ + "question one", + "recovered question", + "recovered question", + "final conclusion", + ] + + +def test_compression_child_stable_ids_remain_after_restamped_parent(monkeypatch): + """Child-sidecar sequence is authoritative even when parent timestamps are later.""" + parent = SimpleNamespace( + session_id="compression-parent", + parent_session_id=None, + session_source="webui", + pre_compression_snapshot=True, + truncation_watermark=None, + truncation_boundary=None, + messages=[ + _user("parent question", 1000.0, id="parent-user"), + _assistant("parent answer", 1400.0, id="parent-answer"), + ], + ) + child = SimpleNamespace( + session_id="compression-child", + parent_session_id="compression-parent", + session_source="webui", + pre_compression_snapshot=False, + truncation_watermark=None, + truncation_boundary=None, + messages=[ + _user("child continuation", 1100.0, id="child-user"), + _assistant("child answer", 1200.0, id="child-answer"), + ], + ) + monkeypatch.setattr( + routes.Session, + "load", + lambda session_id: parent if session_id == parent.session_id else None, + ) + + merged = routes._webui_sidecar_lineage_messages_for_display(child) + + assert [message["id"] for message in merged] == [ + "parent-user", + "parent-answer", + "child-user", + "child-answer", + ]