From 53cabe7e20c9adb4bb98dddb6273700a463ddcbc Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Wed, 6 May 2026 22:45:25 -0700 Subject: [PATCH] fix(session): trim tombstone refs + WHAT-narration in apply-pass comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes round-2 review findings q-1 (minor), q-3 (nit), q-4 (nit), q-5 (nit). * **q-1:** Drop the ``post-migration 050`` clause from the fork-block comment — the apply-pass relocated rather than removed the tombstone-style temporal reference round-1 q-2 was supposed to fix. The bulk-row dict shape and ``_encode_reminders`` are self-explanatory; the WHY is pinned by ``test_fork_preserves_source_and_reminders``. * **q-3:** Replace ``DOES persist now`` framing on the wake-row save comment with a present-tense invariant. The ``now`` implies the reader knows the prior state, same family as the temporal tombstones. * **q-4:** Trim the 12-line WHAT-narration block above the resume-time ``_reminders_delivered = True`` loop to two lines stating the WHY only. The new regression test pins the contract. * **q-5:** Reframe ``test_fork_preserves_source_and_reminders`` docstring as a forward-looking invariant; drop the ``Dropping them was the original bug`` and ``post-migration 050`` fix-narration. Project convention: invariant statements, present tense; don't reference the current task / fix / migration number. (cherry picked from commit b120ee2fd7b5177c9c447dd005a0aea9bb6c677c) --- tests/test_session.py | 14 +++++--------- turnstone/core/session.py | 35 +++++++---------------------------- 2 files changed, 12 insertions(+), 37 deletions(-) diff --git a/tests/test_session.py b/tests/test_session.py index 07c2df19..197cedb8 100644 --- a/tests/test_session.py +++ b/tests/test_session.py @@ -3362,15 +3362,11 @@ class TestReminderSidechannelIsolation: assert "" not in rendered def test_fork_preserves_source_and_reminders(self, tmp_db): - """``ChatSession.resume(..., fork=True)`` bulk-inserts the source - workstream's messages into the fork's own ws_id. The bulk-row - builder must carry the persisted side-channels (``_source`` / - ``_reminders``) — both backends' ``save_messages_bulk`` accept - them post-migration 050. Dropping them was the original bug: - the fork's resumed transcript lost every wake marker and every - reminder bubble that survived to disk on the source, so the - resumed transcript looked like the assistant turn answered out - of nowhere. + """A forked workstream's resumed transcript carries both wake + markers (``_source = "system_nudge"``) and reminder bubbles + (``_reminders``). The bulk-row builder threads the side-channels + onto every fork row so reconnecting tabs see the same shape the + source workstream's originating tab rendered live. """ from turnstone.core.memory import register_workstream, save_message diff --git a/turnstone/core/session.py b/turnstone/core/session.py index f9ae5d81..95b25ab1 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -1744,18 +1744,10 @@ class ChatSession: if not fork: self._ws_id = ws_id self.messages = messages - # Persisted reminders ride the ``_reminders`` side-channel but - # carry no in-memory ``_reminders_delivered`` flag (the flag is - # session-scoped — set by ``_mark_reminders_delivered`` after - # each successful provider stream, never persisted). Without - # this re-splice guard, the very next user turn after resume - # would walk the loaded history and re-render every historical - # ```` block onto the wire — leaking each one a - # second time, the turn after it had already advised. Mirror - # the post-stream hook here: every loaded message that carries - # reminders has already been delivered (it survived to disk), - # so flag it accordingly so ``_apply_reminders_for_provider`` - # short-circuits on the pass-through path. + # Loaded reminders are already delivered — the flag is + # session-scoped and not persisted, so without this guard the + # next user turn would re-splice every historical reminder onto + # the wire. for msg in self.messages: if msg.get("_reminders"): msg["_reminders_delivered"] = True @@ -1855,14 +1847,6 @@ class ChatSession: pd_str = json.dumps(pd) if pd and not isinstance(pd, str) else pd except (TypeError, ValueError): pd_str = None - # Carry the persisted side-channels (``_source`` / - # ``_reminders``) onto the fork's rows. Both backends' - # ``save_messages_bulk`` accept them post-migration 050; - # without them, a fork dropped every wake marker and - # every reminder bubble that survived to disk on the - # source workstream — the resumed fork's transcript - # would then look like the assistant turn answered out - # of nowhere. src = msg.get("_source") bulk_rows.append( { @@ -2786,14 +2770,9 @@ class ChatSession: # leaves pending rows that the UI's chip rehydration can still # surface so the user can clear or resend them. # - # The wake's synthesised empty turn DOES persist now: the - # ``_source`` and ``_reminders`` columns mirror the in-memory - # side-channels so a tab reconnecting via /history sees the - # same system-nudge marker + reminder bubbles the originating - # tab rendered live. Without persistence, multi-tab / multi- - # device replay shows the assistant's response with no - # preceding context — the wake event looks like it came out of - # nowhere. + # The wake's synthesised empty turn carries ``_source`` / + # ``_reminders`` onto the row so reconnecting tabs render the + # marker + bubbles instead of an unanchored assistant reply. source = user_msg.get("_source") reminders_payload = user_msg.get("_reminders") reminders_json = self._encode_reminders(reminders_payload)