fix(state): /undo and /retry work again after a turn that got no reply
A turn that ends with no assistant reply (non-retryable provider error, interrupt) leaves an unanswered user row in the transcript. The next ask makes the live history `user;user`, and the pre-request alternation repair merges the pair into ONE turn in place while both rows stay stored. From then on the warm history has one user turn fewer than the durable projection, and `rewind_user_turn` refused every /undo N and /retry with "session history changed before the rewind could be persisted" for the rest of the session; resume re-merges, so it never healed. Rewind now addresses turns on the same alternation-repaired projection the live process holds (the load `get_resume_conversations` already uses). The merged turn keeps the first row's identity, so /undo 1 soft-deletes both stored rows of the pair and the reply; the in-txn payload pin compares against the STORED first row, so the pin stays strict (no prefix matching). The gateway surface, which has no warm history, gets the same turn boundaries as CLI and TUI instead of undoing only the second half. Slimmer redo of #115511 by @MohamadKanso: same direction (repaired durable load), without the `_unmerged_content` marker on live history dicts and without relaxing `_split_rewind_target` to a startswith match. Co-authored-by: Mohamad Kanso <91088196+MohamadKanso@users.noreply.github.com>
This commit is contained in:
@@ -57,12 +57,17 @@ class SessionRewindMixin:
|
||||
wrong-shape targets raise :class:`RewindTargetUnavailableError`."""
|
||||
from agent.context_compressor import (
|
||||
_DB_PERSISTED_MARKER, history_before_user_originated_turn, retryable_user_text,
|
||||
split_user_originated_turn)
|
||||
split_user_originated_turn, user_originated_turn_view)
|
||||
from agent.message_content import flatten_message_text
|
||||
from agent.session_persistence import _is_ephemeral_scaffolding
|
||||
|
||||
expected_active_ids = self.get_active_message_ids(session_id)
|
||||
durable = self.get_messages_as_conversation(session_id, include_row_ids=True)
|
||||
stored = self.get_messages_as_conversation(session_id, include_row_ids=True)
|
||||
# Live replay (the pre-request repair, a resume) merges a stored ``user;user`` pair — an ask whose turn
|
||||
# ended with no reply, then the next ask — into ONE turn while both rows stay stored. Address turns on
|
||||
# that same repaired projection or the warm history is a turn short of the transcript forever
|
||||
# (#115493); the merged turn keeps the first row's identity, so the rewind starts at that row.
|
||||
durable = self.get_messages_as_conversation(session_id, include_row_ids=True, repair_alternation=True)
|
||||
durable_user = _user_indices(durable)
|
||||
if user_ordinal < 0:
|
||||
user_ordinal = max(len(durable_user) + user_ordinal, 0)
|
||||
@@ -90,10 +95,16 @@ class SessionRewindMixin:
|
||||
target_row_id = target.get("_row_id")
|
||||
if not isinstance(target_row_id, int):
|
||||
raise RuntimeError("rewind target has no durable row identity")
|
||||
# The in-txn payload pin compares against the STORED row, which for a merged turn holds only the
|
||||
# first ask, never the merged text the live views carry.
|
||||
stored_view = next(
|
||||
(user_originated_turn_view(m) for m in stored if m.get("_row_id") == target_row_id), None)
|
||||
if stored_view is None:
|
||||
raise RuntimeError(_HISTORY_CHANGED)
|
||||
try:
|
||||
result = self.rewind_to_message(
|
||||
session_id, target_row_id, preserve_compaction_handoff=scaffold is not None,
|
||||
expected_active_ids=expected_active_ids, expected_target_content=live_view.get("content"))
|
||||
expected_active_ids=expected_active_ids, expected_target_content=stored_view.get("content"))
|
||||
except ValueError as exc: # target vanished / changed role under us: same class of failure as out-of-range
|
||||
raise RewindTargetUnavailableError(str(exc)) from exc
|
||||
if scaffold is not None:
|
||||
|
||||
@@ -120,10 +120,14 @@ def test_rewind_fails_closed_when_new_turn_lands_after_id_snapshot(
|
||||
sibling = SessionDB(db_path=store._db.db_path)
|
||||
original_load = store._db.get_messages_as_conversation
|
||||
|
||||
calls = []
|
||||
|
||||
def _load_then_append(*args, **kwargs):
|
||||
snapshot = original_load(*args, **kwargs)
|
||||
sibling.append_message(sid, "user", "q3-from-other-process")
|
||||
sibling.append_message(sid, "assistant", "a3-from-other-process")
|
||||
if not calls: # one new turn lands between the id snapshot and the write, however often the DB is read
|
||||
sibling.append_message(sid, "user", "q3-from-other-process")
|
||||
sibling.append_message(sid, "assistant", "a3-from-other-process")
|
||||
calls.append(1)
|
||||
return snapshot
|
||||
|
||||
monkeypatch.setattr(store._db, "get_messages_as_conversation", _load_then_append)
|
||||
|
||||
@@ -49,7 +49,7 @@ def _rewind_via(surface: str, db: SessionDB, sid: str, n: int):
|
||||
store._lazy = lambda name, factory: factory()
|
||||
store._clear_dirty_transcript = lambda _sid: None
|
||||
return store.rewind_session(sid, n)
|
||||
warm = db.get_messages_as_conversation(sid)
|
||||
warm = db.get_resume_conversations(sid)[0] # the live process holds the alternation-repaired projection
|
||||
user_turns = sum(1 for m in warm if m.get("role") == "user")
|
||||
ordinal = user_turns - n
|
||||
if surface == "cli":
|
||||
@@ -134,3 +134,20 @@ def test_out_of_range_target_changes_nothing_on_every_surface(db, surface):
|
||||
db.rewind_user_turn(sid + "-empty", -1, warm_history=[])
|
||||
assert _active_rows(db, sid + "-empty") == []
|
||||
assert _active_rows(db, sid) == before
|
||||
|
||||
|
||||
@pytest.mark.parametrize("surface", SURFACES)
|
||||
def test_undo_after_an_unanswered_turn_rewinds_the_merged_turn_on_every_surface(db, surface):
|
||||
"""#115493: a turn that ended with no assistant reply leaves a stored ``user;user`` pair that live replay
|
||||
merges into one turn. /undo 1 takes back that merged turn (both stored rows) instead of refusing with
|
||||
"history changed" forever, and /undo 1 again reaches the turn before it."""
|
||||
sid = f"wedged-{surface}"
|
||||
db.create_session(sid, source="cli")
|
||||
for role, content in (("user", "q1"), ("assistant", "a1"), ("user", "q2_failed"), ("user", "q3"),
|
||||
("assistant", "a3")):
|
||||
db.append_message(sid, role, content)
|
||||
assert len([m for m in db.get_resume_conversations(sid)[0] if m["role"] == "user"]) == 2
|
||||
assert _rewind_via(surface, db, sid, 1) is not None
|
||||
assert [c for _i, _r, c, a in _active_rows(db, sid) if a] == ["q1", "a1"]
|
||||
assert _rewind_via(surface, db, sid, 1) is not None
|
||||
assert [c for _i, _r, c, a in _active_rows(db, sid) if a] == []
|
||||
|
||||
Reference in New Issue
Block a user