From 1ecc728d98878fb35b9761be10b00d524a4bec75 Mon Sep 17 00:00:00 2001 From: Reitzzz Date: Sat, 10 Oct 2026 15:55:16 +0000 Subject: [PATCH 1/2] fix(memory): take the outside result in one query, not a fixed window _last_outside_result scanned only the newest 12 tool results, so a held memory showed no snippet once more than 12 failed or declined calls came after the real outside content (#270). Ask SQLite directly for the newest tool result from an untrusted-content tool that is not an error or decline, so any number of failed calls after the read can never bury it. A CASE guard means a non-JSON or non-object result neither raises nor counts as a failure. Regression test: a real read with 60 declined calls after it, plus a non-JSON result, still recovers the original snippet. --- sentient/tools/builtin/memory_tool.py | 37 ++++++++++++--------------- tests/memory/test_review.py | 19 ++++++++++++++ 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/sentient/tools/builtin/memory_tool.py b/sentient/tools/builtin/memory_tool.py index 1243328d..33975aaf 100644 --- a/sentient/tools/builtin/memory_tool.py +++ b/sentient/tools/builtin/memory_tool.py @@ -6,7 +6,6 @@ from __future__ import annotations -import json import re from sentient.memory import review @@ -74,32 +73,30 @@ 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. + + One query asks SQLite for the newest matching row directly, so any number of failed calls after the real + read never buries it (#270). A result is failed when it is a JSON object carrying ``error`` or ``declined``; + the ``CASE`` guard keeps non-JSON and non-object results (which are never failures) from raising.""" registry = ctx.extra.get("registry") if not ctx.session_id or registry is None or not ctx.untrusted: return None + names = [t.name for t in registry.tools(include_hidden=True) if brings_untrusted(t)] + if not names: + return None + placeholders = ",".join("?" * len(names)) 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,), + row = await ctx.store.fetchone( + f"SELECT content FROM messages WHERE session_id = ? AND role = 'tool' AND name IN ({placeholders})" + " AND NOT (CASE WHEN json_valid(content) THEN" + " COALESCE(json_extract(content, '$.error'), '') != ''" + " OR COALESCE(json_extract(content, '$.declined'), 0) ELSE 0 END)" + " ORDER BY created_at DESC, rowid DESC LIMIT 1", + (ctx.session_id, *names), ) except Exception: 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 - - -def _failed(content: str | None) -> bool: - """A stored tool result that is an error or a declined call (``{"error": ...}`` or ``{"declined": true}``).""" - try: - data = json.loads(content or "") - except ValueError: - return False - return isinstance(data, dict) and bool(data.get("error") or data.get("declined")) + return row["content"] if row is not None else None @tool("memory_forget", risk=Risk.send) diff --git a/tests/memory/test_review.py b/tests/memory/test_review.py index d533cc57..e0df4edc 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,24 @@ 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_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) From 25e025d91d1cf5712b7659095101d1ad097c75bf Mon Sep 17 00:00:00 2001 From: Reitzzz Date: Sun, 11 Oct 2026 03:20:21 +0000 Subject: [PATCH 2/2] fix(memory): keep _failed() truthiness in Python, page the query Addresses review on #283: SQLite's text coercion diverged from the old _failed() semantics -- {"error": false}/{"error": 0}/{"error": {}} were wrongly skipped and {"declined": "yes"} was wrongly accepted. SQL now does only the index-backed part (session, role, name IN, ordering) with no fixed window; _failed() stays in Python and rows are read 50 at a time. Adds a test that a falsey error field is still picked. --- sentient/tools/builtin/memory_tool.py | 44 ++++++++++++++++++--------- tests/memory/test_review.py | 15 +++++++++ 2 files changed, 45 insertions(+), 14 deletions(-) diff --git a/sentient/tools/builtin/memory_tool.py b/sentient/tools/builtin/memory_tool.py index 33975aaf..c2413f87 100644 --- a/sentient/tools/builtin/memory_tool.py +++ b/sentient/tools/builtin/memory_tool.py @@ -6,6 +6,7 @@ from __future__ import annotations +import json import re from sentient.memory import review @@ -75,9 +76,11 @@ 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. - One query asks SQLite for the newest matching row directly, so any number of failed calls after the real - read never buries it (#270). A result is failed when it is a JSON object carrying ``error`` or ``declined``; - the ``CASE`` guard keeps non-JSON and non-object results (which are never failures) from raising.""" + 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 @@ -85,18 +88,31 @@ async def _last_outside_result(ctx: ToolContext) -> str | None: if not names: 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: + """A stored tool result that is an error or a declined call (``{"error": ...}`` or ``{"declined": true}``).""" try: - row = await ctx.store.fetchone( - f"SELECT content FROM messages WHERE session_id = ? AND role = 'tool' AND name IN ({placeholders})" - " AND NOT (CASE WHEN json_valid(content) THEN" - " COALESCE(json_extract(content, '$.error'), '') != ''" - " OR COALESCE(json_extract(content, '$.declined'), 0) ELSE 0 END)" - " ORDER BY created_at DESC, rowid DESC LIMIT 1", - (ctx.session_id, *names), - ) - except Exception: - return None - return row["content"] if row is not None else None + data = json.loads(content or "") + except ValueError: + return False + return isinstance(data, dict) and bool(data.get("error") or data.get("declined")) @tool("memory_forget", risk=Risk.send) diff --git a/tests/memory/test_review.py b/tests/memory/test_review.py index e0df4edc..698d81e3 100644 --- a/tests/memory/test_review.py +++ b/tests/memory/test_review.py @@ -196,6 +196,21 @@ async def test_the_snippet_survives_more_than_a_page_of_failures_after_the_read( 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)