mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
feat(storage): persist tool-result is_error (migration 060)
Tool-result error state was an in-memory-only message key, lost on reload. Add an is_error column to conversations (migration 060, backfilled False) and thread it through the four save layers (memory facade → StorageBackend protocol → SQLite + PostgreSQL): save_message/save_messages_bulk persist it, reconstruct_messages emits it on tool rows, and the session tool-result + synthetic-cancel saves + the fork bulk-copy pass it. It rides as the last conversations column so reconstruct's row-tuple positions stay stable (legacy fixtures default False). history_decoration already prefers the persisted flag over its text heuristic, so reload fidelity improves immediately. First sub-commit of the canonical-trajectory storage cut (folds into rev 060).
This commit is contained in:
@@ -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"]
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
|
||||
Reference in New Issue
Block a user