diff --git a/tests/test_is_error_persistence.py b/tests/test_is_error_persistence.py new file mode 100644 index 00000000..fc9528b4 --- /dev/null +++ b/tests/test_is_error_persistence.py @@ -0,0 +1,60 @@ +"""is_error persistence (canonical-trajectory storage cut #5, sub-commit 1). + +Tool-result error state used to be an in-memory-only message key; it is now a +persisted `conversations.is_error` column so a reload preserves it. These exercise +the round-trip on an ephemeral backend (`_schema` create_all → save → SELECT → +reconstruct); the actual `upgrade()` path is covered by test_migration_060.py. +""" + +from __future__ import annotations + +import json +from typing import Any + +_TC = json.dumps([{"id": "c1", "type": "function", "function": {"name": "x", "arguments": "{}"}}]) + + +def test_tool_is_error_persists(backend: Any) -> None: + ws = "ws-iserr-1" + backend.save_message(ws, "user", "do it") + backend.save_message(ws, "assistant", "", tool_calls=_TC) + backend.save_message(ws, "tool", "boom", tool_call_id="c1", is_error=True) + tool = next(m for m in backend.load_messages(ws, repair=False) if m["role"] == "tool") + assert tool.get("is_error") is True + + +def test_tool_without_error_has_no_flag(backend: Any) -> None: + ws = "ws-iserr-2" + backend.save_message(ws, "tool", "ok", tool_call_id="c1") + tool = next(m for m in backend.load_messages(ws, repair=False) if m["role"] == "tool") + # Only set when True (matches the in-memory convention; consumers use .get()). + assert "is_error" not in tool + + +def test_non_tool_rows_never_carry_is_error(backend: Any) -> None: + ws = "ws-iserr-4" + backend.save_message(ws, "user", "hi") + backend.save_message(ws, "assistant", "hello") + msgs = backend.load_messages(ws, repair=False) + assert all("is_error" not in m for m in msgs) + + +def test_bulk_preserves_is_error(backend: Any) -> None: + ws = "ws-iserr-3" + backend.save_messages_bulk( + [ + { + "ws_id": ws, + "role": "tool", + "content": "boom", + "tool_call_id": "c1", + "is_error": True, + }, + {"ws_id": ws, "role": "tool", "content": "ok", "tool_call_id": "c2"}, + ] + ) + by_id = { + m["tool_call_id"]: m for m in backend.load_messages(ws, repair=False) if m["role"] == "tool" + } + assert by_id["c1"].get("is_error") is True + assert "is_error" not in by_id["c2"] diff --git a/turnstone/core/memory.py b/turnstone/core/memory.py index 04515e73..8859481b 100644 --- a/turnstone/core/memory.py +++ b/turnstone/core/memory.py @@ -43,6 +43,7 @@ def save_message( tool_calls: str | None = None, source: str | None = None, event_id: int | None = None, + is_error: bool = False, ) -> int: """Log a message to the conversations table. @@ -69,6 +70,7 @@ def save_message( tool_calls=tool_calls, source=source, event_id=event_id, + is_error=is_error, ) except Exception: log.warning("Failed to save message for ws=%s role=%s", ws_id, role, exc_info=True) diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 2cef9184..7e995dd0 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -2590,6 +2590,7 @@ class ChatSession: "tool_calls": tc_json, "provider_data": pd_str, "source": src if isinstance(src, str) and src else None, + "is_error": bool(msg.get("is_error", False)), } ) save_messages_bulk(bulk_rows) @@ -4097,7 +4098,8 @@ class ChatSession: "tool_call_id": tc_id, "content": output, } - if self._tool_error_flags.pop(tc_id, False): + tool_is_error = self._tool_error_flags.pop(tc_id, False) + if tool_is_error: tool_msg["is_error"] = True self.messages.append(tool_msg) @@ -4137,6 +4139,7 @@ class ChatSession: _tname, tool_call_id=tc_id, event_id=self._ui_event_id(), + is_error=tool_is_error, ) # Accumulate this result's operator context (guard @@ -4305,6 +4308,7 @@ class ChatSession: func_name, tool_call_id=tc_id, event_id=self._ui_event_id(), + is_error=True, ) # Emit synthetic tool_result so live SSE listeners can # complete the in-DOM tool batch — without this the diff --git a/turnstone/core/storage/_postgresql.py b/turnstone/core/storage/_postgresql.py index a9ec8f72..13c3932b 100644 --- a/turnstone/core/storage/_postgresql.py +++ b/turnstone/core/storage/_postgresql.py @@ -291,6 +291,7 @@ class PostgreSQLBackend: tool_calls: str | None = None, source: str | None = None, event_id: int | None = None, + is_error: bool = False, ) -> int: now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") content = sanitize_text(content) @@ -310,6 +311,7 @@ class PostgreSQLBackend: tool_calls=tool_calls, _source=source, event_id=event_id, + is_error=is_error, ) .returning(conversations.c.id) ) @@ -342,6 +344,7 @@ class PostgreSQLBackend: ), "tool_calls": row.get("tool_calls"), "_source": sanitize_text(row.get("source")), + "is_error": bool(row.get("is_error", False)), } ) with self._conn() as conn: @@ -368,6 +371,7 @@ class PostgreSQLBackend: conversations.c.tool_calls, conversations.c._source, conversations.c.event_id, + conversations.c.is_error, ) .where(conversations.c.ws_id == ws_id) .order_by(conversations.c.id.desc()) @@ -386,6 +390,7 @@ class PostgreSQLBackend: conversations.c.tool_calls, conversations.c._source, conversations.c.event_id, + conversations.c.is_error, ) .where(conversations.c.ws_id == ws_id) .order_by(conversations.c.id) diff --git a/turnstone/core/storage/_protocol.py b/turnstone/core/storage/_protocol.py index c6685f0a..5893105f 100644 --- a/turnstone/core/storage/_protocol.py +++ b/turnstone/core/storage/_protocol.py @@ -159,6 +159,7 @@ class StorageBackend(Protocol): tool_calls: str | None = None, source: str | None = None, event_id: int | None = None, + is_error: bool = False, ) -> int: """Log a message to the conversations table. diff --git a/turnstone/core/storage/_schema.py b/turnstone/core/storage/_schema.py index ebec15d6..9e2d31ba 100644 --- a/turnstone/core/storage/_schema.py +++ b/turnstone/core/storage/_schema.py @@ -56,6 +56,11 @@ conversations = sa.Table( # stay NULL → cursor logic falls back to the snapshot floor. See # migration 059. sa.Column("event_id", sa.BigInteger), + # Tool-result error flag (persisted; migration 060). Set on ``tool`` rows + # whose tool raised or was cancelled, so a reload preserves the error state + # (history rendering + the Anthropic ``is_error`` result block) instead of + # re-deriving it from a text heuristic. Non-tool rows are always False. + sa.Column("is_error", sa.Boolean, nullable=False, server_default=sa.false()), ) sa.Index("idx_conversations_timestamp", conversations.c.timestamp) diff --git a/turnstone/core/storage/_sqlite.py b/turnstone/core/storage/_sqlite.py index 2352edbc..19b3e4eb 100644 --- a/turnstone/core/storage/_sqlite.py +++ b/turnstone/core/storage/_sqlite.py @@ -325,6 +325,7 @@ class SQLiteBackend: tool_calls: str | None = None, source: str | None = None, event_id: int | None = None, + is_error: bool = False, ) -> int: now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S") content = sanitize_text(content) @@ -344,6 +345,7 @@ class SQLiteBackend: "tool_calls": tool_calls, "_source": source, "event_id": event_id, + "is_error": is_error, }, ) if result.lastrowid is None: @@ -390,6 +392,7 @@ class SQLiteBackend: ), "tool_calls": row.get("tool_calls"), "_source": sanitize_text(row.get("source")), + "is_error": bool(row.get("is_error", False)), } ) with self._conn() as conn: @@ -429,6 +432,7 @@ class SQLiteBackend: conversations.c.tool_calls, conversations.c._source, conversations.c.event_id, + conversations.c.is_error, ) .where(conversations.c.ws_id == ws_id) .order_by(conversations.c.id.desc()) @@ -447,6 +451,7 @@ class SQLiteBackend: conversations.c.tool_calls, conversations.c._source, conversations.c.event_id, + conversations.c.is_error, ) .where(conversations.c.ws_id == ws_id) .order_by(conversations.c.id) diff --git a/turnstone/core/storage/_utils.py b/turnstone/core/storage/_utils.py index e254e631..a67d864b 100644 --- a/turnstone/core/storage/_utils.py +++ b/turnstone/core/storage/_utils.py @@ -451,6 +451,9 @@ def reconstruct_messages( # compute the resume cursor + locate the in-flight-turn boundary. # Defensive length check keeps pre-event_id 8-tuple fixtures valid. event_id = row[8] if len(row) > 8 else None + # ``is_error`` (10th column, migration 060) rides last so the tuple + # positions above stay stable; legacy fixtures (≤9-tuples) default False. + is_error = bool(row[9]) if len(row) > 9 else False if role == "user": parts: list[dict[str, Any]] = [] @@ -502,6 +505,8 @@ def reconstruct_messages( "tool_call_id": tc_id or "", "content": content or "", } + if is_error: + tmsg["is_error"] = True if event_id is not None: tmsg["_event_id"] = int(event_id) messages.append(tmsg) diff --git a/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py b/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py index 4904115c..0173a298 100644 --- a/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py +++ b/turnstone/core/storage/migrations/versions/060_unwrap_tool_envelopes.py @@ -159,6 +159,11 @@ def upgrade() -> None: # PostgreSQL (native ALTER) both work — see migration 027. with op.batch_alter_table("conversations") as batch_op: batch_op.drop_column("_reminders") + # Persist the tool-result error flag (was an in-memory-only message key); + # existing rows backfill to False via the server_default. + batch_op.add_column( + sa.Column("is_error", sa.Boolean, nullable=False, server_default=sa.false()) + ) def downgrade() -> None: @@ -168,3 +173,4 @@ def downgrade() -> None: # re-added column is therefore always NULL. with op.batch_alter_table("conversations") as batch_op: batch_op.add_column(sa.Column("_reminders", sa.Text, nullable=True)) + batch_op.drop_column("is_error")