fix(memory): take the outside result in one query, not a fixed window

[Reitzzz]

Summary

_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).

This asks SQLite directly for the newest result from an untrusted-content
tool that isn't an error or decline, so no fixed window can bury it — the
approach you suggested on the issue. A CASE WHEN json_valid guard means a
non-JSON or non-object result neither raises nor counts as a failure.

Testing

Regression test (test_review.py): a real read with 60 declined calls after
it, plus a non-JSON result, still recovers the original snippet.
pytest tests/memory + tests/test_untrusted_content.py → 130 passed.

Closes #270

Summary by CodeRabbit

• Bug Fixes
  • Memory reviews now reliably retain the latest valid result from tools that may return untrusted content, even when later results are declined or unparseable.
  • Valid results are no longer missed simply because many newer tool results are unsuccessful. Falsey error indicators also no longer cause a valid result to be skipped.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 40b1a61d-20ca-429a-b080-68c8b0c6ae51

---

📥 Commits

Reviewing files that changed from the base of the PR and between 1ecc728 and 25e025d.

---

📒 Files selected for processing (2)

• sentient/tools/builtin/memory_tool.py
• tests/memory/test_review.py

---

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

---

📝 Walkthrough

Walkthrough

_last_outside_result now searches eligible untrusted-tool results in newest-first pages of 50. Regression tests cover finding a valid result after more than 12 later results and skipping a result with a truthy declined value.

Changes

Memory review result selection

Layer / File(s)	Summary
Select the newest eligible result sentient/tools/builtin/memory_tool.py, tests/memory/test_review.py	The lookup searches results from registered tools marked as bringing untrusted content, in pages of 50. It returns the first result that _failed does not classify as an error or decline. Tests cover results after 60 declined results and an unparseable result, a falsey error value, and a truthy declined value.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

---

Merge Risk | ⚪ Minimal · up to 25e02

Merge Risk: ⚪ Minimal · up to 25e02

The memory review lookup now finds the newest successful outside result even after many later failed or declined calls. No merge-blocking risk is visible.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 25e02

The change preserves session filtering and the requirement to review memories learned from outside content. No new approval bypass was established. Remaining uncertainty concerns attribution of older snippets and lookup behavior while chat history changes concurrently.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — Outside-content influence on review snippets now extends across eligible historical results in the current chat rather than a fixed recent window. The query remains session-scoped; no additional credential, execution, or cross-session authority is introduced by this selection path.

Security Findings and Attack Paths

• inferred — Untrusted tool content can influence the snippet presented during memory review, but the inspected path does not activate the fact automatically. Exact attribution remains unproven: selection uses the latest eligible result, not a producing-call identity bound to the particular fact. This attribution limitation predates the PR, although the searchable history is broader.

Trust Boundaries and Controls

• observed — Outside-content context creates a review object before lookup. Approval, discard, and expiration initially select pending facts, while review routes declare the existing AUTH dependency. These controls are unchanged by the PR; their presence does not establish deployment-level tenant isolation.

Resilience and Maintainability Implications

• inferred — Exhaustion or a database exception yields an absent snippet without removing the hold. Interruption during lookup occurs before remembering begins, and any subsequent held insert is explicitly pending. Pagination has no explicit cross-page snapshot, so concurrent changes may affect snippet selection and processing time; this uncertainty does not demonstrate an approval bypass.

Hardening Proposals

• proposed — As optional provenance strengthening, retain the producing tool-call identity with each held fact instead of treating the latest eligible session result as exact evidence for that fact. This addresses a pre-existing attribution limitation, not a verified vulnerability introduced here.

---

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly describes the main change: replacing the fixed result window used by _last_outside_result.
Linked Issues check		Issue #270 requires the code to find the valid outside result after at least 13 later failed results. _last_outside_result now queries eligible tool messages in newest-first order and reads pages of…
Out of Scope Changes check		The changes stay within issue #270. The query removes the fixed result window, and the added tests verify paging and preserved failure truthiness. No unrelated production or test changes are shown.
Docstring Coverage		Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.

---

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{A rabbit checks the newest trail,
Through pages fifty, without fail.
A falsey error lets results through,
A declined result is skipped too.
The older snippet stays in view,
Then hops away beneath the moon.}

_{Comment @coderabbitai help to get the list of available commands.}

[Reitzzz]

I have read the CLA Document and I hereby sign the CLA

[itsskofficial]

Thanks @Reitzzz for picking this up, the fix works! I ran it for real on an engine from your branch: one approved page read, then 16 declined browser_open calls, then memory_remember. The held memory's snippet was the page text, and on the same database the old 12-row window returned nothing. Your new test also fails on the old code, which is great. The query uses the session index too, so it stays fast even on very long chats: about 10 ms under 20,000 declines.

One thing before merge: the SQL check for "failed" doesn't decide things quite the way the old _failed() did, which used Python truthiness. Two cases:

• Now skipped wrongly. SQLite treats 0 != '' as true and returns {} and [] as non-empty text, so a successful result like {"error": false, "data": ...}, {"error": 0} or {"error": {}} is now skipped as failed.
• Now accepted wrongly. {"declined": "yes"} or {"declined": {...}} counts as a success, because SQLite reads that text as 0.

The engine's own errors and declines still match. But MCP and web results can carry an error key with any value.

Simplest fix: let SQL do only the cheap part (session, role, the name IN list and the ordering, which uses the index) and keep _failed() in Python. Read rows in pages, say 50 at a time with LIMIT ? OFFSET ?, and return the first one _failed() accepts. That keeps your #270 fix with no fixed window and keeps the old meaning exactly. Please also add a test where a successful result has "error": false and is still picked, and update the docstring to match.

[Reitzzz]

Thanks for the thorough review and for actually running it against an engine — good catch on the truthiness mismatch.

Pushed 25e025d9. SQL now does only the cheap, index-backed part (session, role = 'tool', the name IN allow-list, and the newest-first ordering) with no fixed window. The success/failure decision moves back into Python via _failed(), so the exact old semantics are preserved: {"error": false} / {"error": 0} / {"error": {}} are successes, and {"declined": "yes"} is a decline. Rows are read in pages of 50 with LIMIT ? OFFSET ?, returning the first result _failed() accepts, so a long chat never loads at once and #270's no-fixed-window fix stands.

Added test_a_falsey_error_field_is_still_a_successful_result: a result with "error": false is still picked over a newer {"declined": "yes"}. Both this and the #270 paging test fail on the old SQL. Updated the docstring to describe the SQL/Python split.

CI is green (Engine tests ubuntu + windows, Lint, Desktop build, Secret scan) and CodeRabbit found no actionable comments.

[itsskofficial]

Approved after a real end-to-end test.

[itsskofficial]

Merged, thanks @Reitzzz! I re-ran the real test on 25e025d9: in a chat with one approved page read followed by 16 declined browser_open calls, a new memory_remember was held for review with the page text as its snippet. The old 12-row window returned nothing on that same database. Paging with _failed() in Python is exactly right. Great first contribution!