diff --git a/sentient/tools/builtin/memory_tool.py b/sentient/tools/builtin/memory_tool.py index 1243328d..c2413f87 100644 --- a/sentient/tools/builtin/memory_tool.py +++ b/sentient/tools/builtin/memory_tool.py @@ -74,23 +74,36 @@ async def _mark_if_outside(ctx: ToolContext, session_ids: list) -> None: async def _last_outside_result(ctx: ToolContext) -> str | None: """The latest successful tool result in this chat that brought in outside content (the review card shows it). - Errors and declined calls are skipped: they say nothing about where a fact came from.""" + Errors and declined calls are skipped: they say nothing about where a fact came from. + + SQL does only the cheap, index-backed part (session, tool role, the ``name IN`` allow-list and the newest-first + ordering); it carries no fixed window, so any number of failed calls after the real read never buries it (#270). + The success/failure decision stays in Python (``_failed``), preserving the exact truthiness semantics the old + code had: ``{"error": false}`` / ``{"error": 0}`` / ``{"error": {}}`` are successes, and ``{"declined": "yes"}`` + is a decline. Rows are read in pages so a long chat never loads at once.""" registry = ctx.extra.get("registry") if not ctx.session_id or registry is None or not ctx.untrusted: return None - try: - rows = await ctx.store.fetchall( - "SELECT name, content FROM messages WHERE session_id = ? AND role = 'tool'" - " ORDER BY created_at DESC, rowid DESC LIMIT 12", - (ctx.session_id,), - ) - except Exception: + names = [t.name for t in registry.tools(include_hidden=True) if brings_untrusted(t)] + if not names: return None - for r in rows: - t = registry.get(r["name"] or "") - if t is not None and brings_untrusted(t) and not _failed(r["content"]): - return r["content"] - return None + placeholders = ",".join("?" * len(names)) + sql = ( + f"SELECT content FROM messages WHERE session_id = ? AND role = 'tool' AND name IN ({placeholders})" + " ORDER BY created_at DESC, rowid DESC LIMIT ? OFFSET ?" + ) + offset = 0 + while True: + try: + rows = await ctx.store.fetchall(sql, (ctx.session_id, *names, 50, offset)) + except Exception: + return None + if not rows: + return None + for r in rows: + if not _failed(r["content"]): + return r["content"] + offset += len(rows) def _failed(content: str | None) -> bool: diff --git a/tests/memory/test_review.py b/tests/memory/test_review.py index d533cc57..698d81e3 100644 --- a/tests/memory/test_review.py +++ b/tests/memory/test_review.py @@ -3,6 +3,7 @@ from __future__ import annotations import functools +import json from datetime import UTC, datetime, timedelta import pytest @@ -177,6 +178,39 @@ async def test_the_review_snippet_is_the_content_read_not_a_later_error_or_decli assert "declined" not in snippet and "NOT DONE" not in snippet and "No email" not in snippet +async def test_the_snippet_survives_more_than_a_page_of_failures_after_the_read(chat): + """#270: the real snippet must survive any number of failed calls after it. + + One SQL query takes the newest non-failed outside result, so no fixed window buries it. A non-JSON + result mixed in among the failures must not raise and must not be mistaken for a failure.""" + from sentient.tools.builtin.memory_tool import _last_outside_result + + sid = await chat.store.create_session(channel="cli") + good = json.dumps({"from": "travel@example.com", "body": EMAIL}) + await chat.store.add_message(sid, "tool", "Not JSON at all.", name="mail_read") # predicate must not raise on it + await chat.store.add_message(sid, "tool", good, name="mail_read") + for _ in range(60): # far past the old fixed 12-row window and any single page + await chat.store.add_message(sid, "tool", json.dumps(DECLINED), name="mail_read") + ctx = chat.agent.tool_context(sid, "cli") + ctx.untrusted = "Mail" + assert await _last_outside_result(ctx) == good + + +async def test_a_falsey_error_field_is_still_a_successful_result(chat): + """A successful result may carry ``error`` with a falsey value (``false``/``0``/``{}``); it must still be + picked, and a decline whose ``declined`` reads as a non-empty string must still be skipped. Python truthiness + decides this, not SQLite's text coercion (which read ``false``/``{}`` as failures and ``"yes"`` as a success).""" + from sentient.tools.builtin.memory_tool import _last_outside_result + + sid = await chat.store.create_session(channel="cli") + good = json.dumps({"from": "travel@example.com", "body": EMAIL, "error": False}) # falsey error == success + await chat.store.add_message(sid, "tool", good, name="mail_read") + await chat.store.add_message(sid, "tool", json.dumps({"declined": "yes"}), name="mail_read") # newest, a decline + ctx = chat.agent.tool_context(sid, "cli") + ctx.untrusted = "Mail" + assert await _last_outside_result(ctx) == good + + async def test_work_nobody_asked_for_saves_memories_for_review(chat): for origin, label in (("proactive", "a proactive check"), ("heartbeat", "a background check")): ctx = chat.agent.tool_context(None, origin)