From 63e9205e83f4eef37c04486a02efae8d74efe6bc Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Wed, 3 Jun 2026 02:48:15 -0700 Subject: [PATCH] refactor(storage): drop the load-time orphan synth; repair only at send MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconstruct_messages(repair=True) did two things: strip a trailing incomplete tool-call turn AND synthesize cancellation results for mid-conversation orphans. The mid-orphan synth was a near-duplicate of lowering.repair_wire_messages — same detector, same contiguous insert past interspersed system turns, same cancellation string — running at the wrong layer (storage, on every load). Drop it: load is now trailing-strip only (boot-crash recovery), and the mid-orphan synth happens once, at send, in lowering.repair_wire_messages — the single place the wire path fills orphans. The session send path gets it via _prepare_wire_messages; export, which bypasses that path, now runs repair_wire_messages itself (otherwise a mid-conversation orphan would serialize as an unanswered tool_call). The duplicated cancellation string goes with the synth — CANCELLED_TOOL_RESULT lives only in lowering now. Safe: a bare mid-orphan is harmless between load and send (token count is additive, /history reads repair=False, compaction summarizes to text), and every wire path repairs it. Reconstruct tests updated to the new load contract; an export mid-orphan test added. --- tests/test_export.py | 18 +++++++ tests/test_reconstruct_messages.py | 59 ++++++++------------ turnstone/core/export.py | 9 ++-- turnstone/core/storage/_utils.py | 86 +++++++----------------------- 4 files changed, 66 insertions(+), 106 deletions(-) diff --git a/tests/test_export.py b/tests/test_export.py index 08988589..13f15e20 100644 --- a/tests/test_export.py +++ b/tests/test_export.py @@ -194,3 +194,21 @@ def test_attach_reasoning_runs_before_sanitize(backend): leaked = [k for m in messages for k in m if isinstance(k, str) and k.startswith("_")] assert leaked == [] assert _assistants(messages)[0].get("reasoning_content") == "R1" + + +def test_mid_orphan_tool_call_exports_with_cancellation(backend): + """A mid-conversation orphaned tool_call (no result) exports with a + synthesized cancellation: export bypasses the session send path, so it runs + the send-time orphan repair itself (load is trailing-strip only).""" + tc = [{"id": "call_x", "type": "function", "function": {"name": "run", "arguments": "{}"}}] + backend.register_workstream("ws1", user_id=USER, title="T", kind="interactive") + backend.save_message("ws1", "user", "go") + backend.save_message("ws1", "assistant", "working", tool_calls=json.dumps(tc)) + # A user turn after the orphan keeps it mid-conversation (not stripped). + backend.save_message("ws1", "user", "never mind") + + messages = _parse_messages(_build_openai_json(backend, "ws1")) + tool_msgs = [m for m in messages if m.get("role") == "tool"] + assert len(tool_msgs) == 1 + assert tool_msgs[0]["tool_call_id"] == "call_x" + assert "cancelled" in tool_msgs[0]["content"].lower() diff --git a/tests/test_reconstruct_messages.py b/tests/test_reconstruct_messages.py index db7a1ff1..8d294cb8 100644 --- a/tests/test_reconstruct_messages.py +++ b/tests/test_reconstruct_messages.py @@ -249,10 +249,12 @@ class TestEdgeCases: class TestMidConversationOrphanRepair: - """Mid-conversation orphaned tool_calls get synthetic tool results.""" + """Mid-conversation orphaned tool_calls are LEFT bare at load — the + send-time repair (``lowering.repair_wire_messages``, covered in + ``test_lowering.py``) fills them. Load is trailing-strip only.""" - def test_all_orphaned_mid_conversation(self): - """Assistant has 2 tool_calls, no tool results, then user message.""" + def test_all_orphaned_not_synthesized_at_load(self): + """Assistant has 2 unanswered tool_calls then a user turn: left bare.""" tc = json.dumps( [ {"id": "c1", "function": {"name": "bash", "arguments": "{}"}}, @@ -265,17 +267,13 @@ class TestMidConversationOrphanRepair: _row("user", "never mind"), ] msgs = reconstruct_messages(rows, "ws1") - # Should have: user, assistant, tool(c1), tool(c2), user - assert len(msgs) == 5 - assert msgs[2]["role"] == "tool" - assert msgs[2]["tool_call_id"] == "c1" - assert msgs[2]["is_error"] is True - assert msgs[3]["role"] == "tool" - assert msgs[3]["tool_call_id"] == "c2" - assert msgs[4]["role"] == "user" + # No synthesis at load — the orphan is mid-conversation (not trailing), + # so the strip leaves it; the send pass synthesizes it. + assert [m["role"] for m in msgs] == ["user", "assistant", "user"] + assert not any(m["role"] == "tool" for m in msgs) - def test_partial_results_mid_conversation(self): - """2 tool_calls, 1 result present, 1 missing — synthesize only the missing one.""" + def test_partial_results_not_synthesized_at_load(self): + """A present result is kept; the missing sibling is NOT filled at load.""" tc = json.dumps( [ {"id": "c1", "function": {"name": "bash", "arguments": "{}"}}, @@ -289,16 +287,12 @@ class TestMidConversationOrphanRepair: _row("user", "skip the write"), ] msgs = reconstruct_messages(rows, "ws1") - # Should have: user, assistant, tool(c1 real), tool(c2 synthetic), user - assert len(msgs) == 5 - assert msgs[2]["role"] == "tool" - assert msgs[2]["tool_call_id"] == "c1" - assert msgs[2]["content"] == "file1.txt" - assert msgs[2].get("is_error") is not True - assert msgs[3]["role"] == "tool" - assert msgs[3]["tool_call_id"] == "c2" - assert msgs[3]["is_error"] is True - assert msgs[4]["role"] == "user" + # Real c1 result kept; c2 left orphaned (synthesized at send, not here). + assert [m["role"] for m in msgs] == ["user", "assistant", "tool", "user"] + tool_msgs = [m for m in msgs if m["role"] == "tool"] + assert len(tool_msgs) == 1 + assert tool_msgs[0]["tool_call_id"] == "c1" + assert tool_msgs[0]["content"] == "file1.txt" def test_complete_results_no_synthesis(self): """All tool_calls have results — no synthesis needed.""" @@ -429,10 +423,10 @@ class TestSystemTurns: assert tool_msgs[0].get("is_error") is not True assert [m["role"] for m in msgs] == ["user", "assistant", "system", "tool", "user"] - def test_synthetic_result_stays_adjacent_to_real_results_past_system(self): - """When a call IS orphaned and a system turn follows the real results, - the synthetic is inserted adjacent to the real block (before the system - turn), keeping the tool-result block contiguous.""" + def test_orphan_past_system_left_bare_at_load(self): + """When a call IS orphaned with a system turn after the real results, + load leaves it bare (no splice). The send-time repair's contiguous + insertion past system turns is covered in ``test_lowering.py``.""" tc = json.dumps( [ {"id": "c1", "function": {"name": "bash", "arguments": "{}"}}, @@ -447,16 +441,9 @@ class TestSystemTurns: _row("user", "skip c2"), # c2 never resulted ] msgs = reconstruct_messages(rows, "ws1") - assert [m["role"] for m in msgs] == [ - "user", - "assistant", - "tool", - "tool", - "system", - "user", - ] + # No synthetic c2 at load — only the real c1 result. + assert [m["role"] for m in msgs] == ["user", "assistant", "tool", "system", "user"] assert msgs[2]["tool_call_id"] == "c1" and msgs[2].get("is_error") is not True - assert msgs[3]["tool_call_id"] == "c2" and msgs[3]["is_error"] is True _PNG = ( diff --git a/turnstone/core/export.py b/turnstone/core/export.py index 2b02fded..47054fa9 100644 --- a/turnstone/core/export.py +++ b/turnstone/core/export.py @@ -32,6 +32,7 @@ from typing import TYPE_CHECKING, Any from turnstone.core.history_decoration import ( extract_reasoning_text_from_provider_content, ) +from turnstone.core.lowering import repair_wire_messages from turnstone.core.providers._openai_common import sanitize_messages if TYPE_CHECKING: @@ -79,9 +80,11 @@ def _attach_reasoning_content(messages: list[dict[str, Any]]) -> list[dict[str, def _build_openai_json(storage: StorageBackend, ws_id: str) -> bytes: """Serialize one workstream's history as an OpenAI envelope (JSON bytes).""" - messages = sanitize_messages( - _attach_reasoning_content(storage.load_messages(ws_id, repair=True)) - ) + # Export bypasses the session send path, so it runs the send-time orphan + # repair itself (``load`` only strips the trailing turn) — otherwise a + # mid-conversation orphaned tool_call would serialize as an unanswered call. + loaded = _attach_reasoning_content(storage.load_messages(ws_id, repair=True)) + messages = sanitize_messages(repair_wire_messages(loaded)) return json.dumps({"messages": messages}, ensure_ascii=False, indent=2).encode("utf-8") diff --git a/turnstone/core/storage/_utils.py b/turnstone/core/storage/_utils.py index fc5a9ab2..b15fc49b 100644 --- a/turnstone/core/storage/_utils.py +++ b/turnstone/core/storage/_utils.py @@ -535,18 +535,19 @@ def reconstruct_messages( (``read_file`` on an image) this way — they would otherwise reload as the flattened text alone. - When ``repair`` is True (default) the result is post-processed to - produce a wire-shape valid for an LLM round-trip: the trailing - ``assistant(tool_calls)`` turn is dropped if not all tool_call ids - have a matching tool result (trailing operator-context ``system`` - turns are looked through and stripped with it), and any - mid-conversation orphaned tool_calls are filled with synthetic - cancellation results. Callers - that consume the messages as LLM context (e.g. ``session.resume``) - must keep this on. Callers reading for *display* (the ``/history`` - REST endpoint) should pass ``repair=False`` so the user sees the - actual partial state — refreshing during tool execution otherwise - silently drops the trailing turn from the UI. + When ``repair`` is True (default) the trailing ``assistant(tool_calls)`` + turn is dropped if not all tool_call ids have a matching tool result + (trailing operator-context ``system`` turns are looked through and stripped + with it) — boot-crash recovery so a half-finished turn never replays. + Mid-conversation orphaned tool_calls are *not* filled here; that is the + send-time repair (:func:`turnstone.core.lowering.repair_wire_messages`), the + single place the wire path synthesizes cancellation results. Callers that + consume the messages as LLM context via the session send path + (``session.resume``) get that repair for free; a consumer that bypasses it + (``export``) runs ``repair_wire_messages`` itself. Callers reading for + *display* (the ``/history`` REST endpoint) should pass ``repair=False`` so + the user sees the actual partial state — refreshing during tool execution + otherwise silently drops the trailing turn from the UI. """ messages: list[dict[str, Any]] = [] for row in rows: @@ -686,59 +687,10 @@ def reconstruct_messages( break del messages[asst_idx:] - # Repair: synthesize tool results for mid-conversation orphaned tool calls. - # This happens when a cancel interrupts tool execution — the assistant - # message with tool_calls is saved to DB but GenerationCancelled prevents - # tool results from being created. Both Anthropic (strict) and OpenAI - # (lenient today, may tighten) benefit from well-formed histories. - i = 0 - while i < len(messages): - msg = messages[i] - if msg.get("role") == "assistant" and msg.get("tool_calls"): - expected_ids = [tc.get("id", "") for tc in msg["tool_calls"] if tc.get("id")] - # Collect the tool-result ids that follow, looking *through* any - # operator-context system/developer turns interspersed in the block - # (they follow the turn they relate to and must not be mistaken for - # the end of the tool-result run — the same skip pass 1 applies). - # ``insert_at`` tracks the slot right after the last real tool - # result so synthesized results stay contiguous with the real ones - # (Anthropic requires every tool_result adjacent to its tool_use); - # without this, a synthetic spliced after a trailing system turn - # would split the block. - j = i + 1 - result_ids: set[str] = set() - insert_at = i + 1 - while j < len(messages) and messages[j].get("role") in ( - "tool", - "system", - "developer", - ): - if messages[j].get("role") == "tool": - tc_id = messages[j].get("tool_call_id", "") - if tc_id: - result_ids.add(tc_id) - insert_at = j + 1 - j += 1 - # Synthesize results for any missing IDs - orphaned = [uid for uid in expected_ids if uid not in result_ids] - if orphaned: - synthetic = [ - { - "role": "tool", - "tool_call_id": uid, - "content": "Tool execution was cancelled.", - "is_error": True, - } - for uid in orphaned - ] - messages[insert_at:insert_at] = synthetic - if orphaned: - i = j + len(orphaned) # skip past the (now longer) block - elif j > i + 1: - i = j # skip past existing tool block - else: - i += 1 # no tools followed; just advance - else: - i += 1 - + # Mid-conversation orphaned tool_calls are NOT synthesized here — that is the + # send-time repair (``lowering.repair_wire_messages``), the single place the + # wire path fills them. Load stays trailing-strip-only: the bare orphan is + # harmless between load and send (token-count is additive, ``/history`` reads + # ``repair=False``, compaction summarizes to text), and the send pass repairs + # it. Non-session consumers (``export``) run ``repair_wire_messages`` too. return messages