fix(chat): restore history after failed or cancelled turns

[kovtcharov]

Failed requests and cancelled streams no longer leave unanswered messages or retrieved passages in conversation history. Rollback restores entries evicted from a full history buffer; closing at the final streamed chunk preserves the completed turn.

Fixes #3512.

Test plan:

• Nine focused regressions cover formatter/generation failures, cancellation, mid-stream errors, successful commits, and closing immediately at the final chunk.
• Eight existing live Lemonade integration tests passed, including conversation memory and streaming.
• Black, isort, whitespace checks, code and architecture review passed.

[github-actions]

Verdict: Approve with suggestions

A failed or cancelled chat turn no longer leaves the user's unanswered question (and any retrieved RAG passages) stuck in the conversation, and the rollback correctly brings back entries that the bounded history buffer had already evicted. The fix is small, the ordering is right, and the new tests cover the cases that matter.

The one thing worth doing before merge: the whole fix hinges on a subtle ordering detail — the turn is marked finished just before the final chunk is handed to the caller — and nothing in the test suite pins that down. Several callers (the CLI, Telegram, the voice SDK) stop reading as soon as they see the final chunk, so if someone later moves that one line, those callers would silently start losing history again while every test stayed green. One extra test closes that door.

Smaller points: the same "snapshot and restore" block now appears three times in this one file and is worth pulling into a helper, the two public methods still don't mention the new guarantee in their own docs, and the new documentation note landed inside an unrelated example rather than in the section about conversation history.

No security concerns.

Real-world evidence

N/A — no evidence bundle was produced for this run, so my verdict rests on static review plus a standalone replication of the new control flow. I could not execute the repo's tests in this environment (the runner is missing the project's Python dependencies), so I am not claiming they pass; the PR description states the new unit tests and eight live Lemonade integration tests were run locally. The change is SDK-internal with no new CLI flag, route, or UI surface, so there are no pixels or endpoints pending.

What I did verify directly: I replicated the exact generator/finally structure in an isolated script and confirmed all three consumer patterns behave as intended — full consumption commits the turn, a consumer that stops at the final chunk keeps its history, and a mid-stream cancel rolls back.

full:            ['user: q', 'assistant: done']
break-at-final:  ['user: q', 'assistant: done']
cancel-mid:      ['a', 'b']

🔍 Technical details

Issues found

🟢 The completed-before-yield invariant is untested (src/gaia/chat/sdk.py:619)

completed = True is deliberately set before yield result so a consumer that breaks on is_complete=True triggers GeneratorExit with the turn already committed. That ordering is what keeps src/gaia/cli.py:437, src/gaia/messaging/telegram.py:214, src/gaia/talk/sdk.py:185 and sdk.py:801 working — they all read to the final chunk, and a future refactor that moves the assignment after the yield would roll back a successful turn with no test failing. test_success_records_original_user_text_and_complete_reply exhausts the generator via list(...), which doesn't exercise the close-at-final-yield path.

def test_history_survives_break_at_final_chunk(sdk):
    sdk.llm_client.generate.return_value = iter(["one", "two"])
    for chunk in sdk.send_stream("new question"):
        if chunk.is_complete:
            break
    assert list(sdk.chat_history) == ["user: new question", "assistant: onetwo"]

🟢 Third copy of the snapshot/restore block (sdk.py:544, sdk.py:625, sdk.py:1002)

clear() + extend(original_history) now appears three times in this file (_summarize_history already had its own copy at sdk.py:1002-1011). A small helper removes the drift risk and makes the commit point explicit at each call site:

@contextmanager
def _history_rollback(self):
    """Restore the pre-turn history unless the caller commits."""
    snapshot = list(self.chat_history)
    turn = SimpleNamespace(committed=False)
    try:
        yield turn
    finally:
        if not turn.committed:
            self.chat_history.clear()
            self.chat_history.extend(snapshot)

Call sites become with self._history_rollback() as turn: … turn.committed = True immediately before the return / final yield, which also keeps the ordering invariant above visible at the point it matters.

🟢 New guarantee is absent from the method docstrings (sdk.py:458-468, sdk.py:550-558)

send()'s docstring still only promises "AgentResponse with the complete response and updated history". The rollback is a contract change integrators should be able to see from the docstring, which is the SDK's primary reference surface:

        Returns:
            AgentResponse with the complete response and updated history.
            If the turn fails, the conversation history is left exactly as it
            was before the call.

🟢 Doc note landed in the wrong section (docs/sdk/sdks/chat.mdx:116)

The new line sits as a second stacked comment above # Check history inside the multi-turn example, where it reads as a comment about get_history(). docs/sdk/sdks/chat.mdx:188 already has a dedicated ## Conversation History section — a sentence of prose there is more discoverable than a comment in an unrelated snippet.

🟢 Rollback assumes one turn at a time per instance

The restore is a whole-buffer clear()/extend(), so if anything mutates chat_history between the snapshot and a failure (a second concurrent turn on a shared instance, or a clear_history() call during an in-flight stream), the failing turn's rollback undoes that too. Every caller I checked is single-turn-per-instance (Telegram uses per-user sessions), so this isn't a live bug — just worth knowing if the SDK is ever shared across requests.

Strengths

• The evicted-entry case is handled, not just the dangling message. Snapshotting the full deque before the append means a maxlen-full history gets its oldest entry back on failure; a naive pop() fix would have silently dropped it. The tests assert it (maxlen == 2, identity of the deque preserved).
• Good coverage of the hard paths for a generator-based API: failure before generation, failure after a chunk has already been yielded, and explicit close() cancellation — parametrized across both send and send_stream.
• finally rather than except means KeyboardInterrupt and GeneratorExit are covered too, which is exactly what the interactive session at sdk.py:801 and the Agent UI stop button need. Reusing the clear()/extend() shape already present in _summarize_history keeps it consistent with the file.