test(memory): cover recall-count clamp/fallback; extract resolve_memory_recall_count helper (#4948)

Adopts fresh-review nit: the new robustness logic (malformed-pref fallback +
[1,50] clamp) had no test coverage. Extracted the inline pref resolution into a
pure resolve_memory_recall_count() helper and added cases for None/NaN/string/
negative/oversized/float inputs. Behaviour unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: maxmilian

routes/chat_helpers.py

@@ -38,6 +38,24 @@
 )
 
 
+MEMORY_RECALL_COUNT_MIN = 1
+MEMORY_RECALL_COUNT_MAX = 50
+
+
+def resolve_memory_recall_count(uprefs: dict) -> int:
+    """Number of extended memories recalled into context per response (issue #4948).
+
+    Resolves user pref -> global setting -> the historical default of 3, coerces to
+    int, and clamps to [1, 50] so a malformed pref (NaN, negative, string, null)
+    can't silently disable recall (k<=0) or balloon the prompt.
+    """
+    try:
+        count = int(uprefs.get("memory_recall_count", get_setting("memory_recall_count", 3)))
+    except (TypeError, ValueError):
+        count = 3
+    return max(MEMORY_RECALL_COUNT_MIN, min(MEMORY_RECALL_COUNT_MAX, count))
+
+
 def _is_casual_low_signal(text: str) -> bool:
     """Short greetings/slang should not pull memory, skills, RAG, or docs."""
     s = str(text or "").strip()
@@ -692,14 +710,7 @@ async def build_chat_context(
     # Skills injection respects its own enable toggle (mirrors memory_enabled).
     # When off, the "Available skills" index is not added to the prompt.
     skills_enabled = not incognito and uprefs.get("skills_enabled", True)
-    # Number of extended memories recalled per response (issue #4948): user pref,
-    # then global setting, then the historical default of 3. Clamped so a
-    # malformed pref can't silently disable recall (k<=0) or balloon the prompt.
-    try:
-        mem_recall_count = int(uprefs.get("memory_recall_count", get_setting("memory_recall_count", 3)))
-    except (TypeError, ValueError):
-        mem_recall_count = 3
-    mem_recall_count = max(1, min(50, mem_recall_count))
+    mem_recall_count = resolve_memory_recall_count(uprefs)
     if not allow_tool_preprocessing:
         mem_enabled = False
         skills_enabled = False

tests/test_memory_recall_count_4948.py

@@ -2,45 +2,55 @@
 
 `build_context_preface` retrieves a number of extended (non-pinned) memories to
 inject into the model context. That count used to be hardcoded; it is now driven
-by the `mem_recall_count` parameter so a user setting can control it.
+by the `mem_recall_count` parameter (resolved from a user pref) so a setting can
+control it.
 """
 from types import SimpleNamespace
 from unittest.mock import MagicMock
 
+import pytest
+
 from src.chat_processor import ChatProcessor
+from routes.chat_helpers import (
+    resolve_memory_recall_count,
+    MEMORY_RECALL_COUNT_MIN,
+    MEMORY_RECALL_COUNT_MAX,
+)
 
 
-def _processor_with_memories(monkeypatch, n_extended=12):
-    """Build a ChatProcessor whose memory store returns `n_extended` non-pinned memories."""
+def _processor_with_memories(n_extended=12):
+    """A ChatProcessor whose memory store returns `n_extended` non-pinned memories."""
     mem_entries = [
         {"id": str(i), "text": f"fact {i}", "category": "fact", "pinned": False}
         for i in range(n_extended)
     ]
     memory_manager = MagicMock()
     memory_manager.load.return_value = mem_entries
-    processor = ChatProcessor(memory_manager=memory_manager, personal_docs_manager=MagicMock())
-    return processor
+    return ChatProcessor(memory_manager=memory_manager, personal_docs_manager=MagicMock())
 
 
-def test_mem_recall_count_drives_retrieval_k(monkeypatch):
-    """A non-default mem_recall_count is forwarded to the retrieval step as k."""
-    processor = _processor_with_memories(monkeypatch)
+def _capture_k(monkeypatch, processor):
     captured = {}
 
-    def fake_retrieve(message, mem_entries, k=5):
+    def fake_retrieve(message, mem_entries, k):
         captured["k"] = k
         return []
 
     monkeypatch.setattr(processor, "_hybrid_retrieve", fake_retrieve)
+    return captured
+
+
+# ── build_context_preface forwards the count to retrieval ───────────────────
+
+def test_mem_recall_count_drives_retrieval_k(monkeypatch):
+    """A non-default mem_recall_count is forwarded to the retrieval step as k."""
+    processor = _processor_with_memories()
+    captured = _capture_k(monkeypatch, processor)
     session = SimpleNamespace(endpoint_url="http://local", model="test", headers={})
 
     processor.build_context_preface(
-        message="hello",
-        session=session,
-        use_web=False,
-        use_rag=False,
-        use_memory=True,
-        use_skills=False,
+        message="hello", session=session,
+        use_web=False, use_rag=False, use_memory=True, use_skills=False,
         mem_recall_count=7,
     )
 
@@ -49,23 +59,44 @@ def fake_retrieve(message, mem_entries, k=5):
 
 def test_mem_recall_count_defaults_to_current_behavior(monkeypatch):
     """Omitting mem_recall_count preserves the previous hardcoded count (3)."""
-    processor = _processor_with_memories(monkeypatch)
-    captured = {}
-
-    def fake_retrieve(message, mem_entries, k=5):
-        captured["k"] = k
-        return []
-
-    monkeypatch.setattr(processor, "_hybrid_retrieve", fake_retrieve)
+    processor = _processor_with_memories()
+    captured = _capture_k(monkeypatch, processor)
     session = SimpleNamespace(endpoint_url="http://local", model="test", headers={})
 
     processor.build_context_preface(
-        message="hello",
-        session=session,
-        use_web=False,
-        use_rag=False,
-        use_memory=True,
-        use_skills=False,
+        message="hello", session=session,
+        use_web=False, use_rag=False, use_memory=True, use_skills=False,
     )
 
     assert captured.get("k") == 3
+
+
+# ── resolve_memory_recall_count: pref resolution + clamp + malformed input ──
+
+def test_resolve_uses_valid_pref():
+    assert resolve_memory_recall_count({"memory_recall_count": 10}) == 10
+
+
+def test_resolve_missing_pref_falls_back_to_default():
+    # No pref -> global setting -> historical default of 3.
+    assert resolve_memory_recall_count({}) == 3
+
+
+@pytest.mark.parametrize("bad", [None, "abc", float("nan"), [1], {}])
+def test_resolve_malformed_pref_falls_back_to_3(bad):
+    """A non-int / NaN / null pref must not throw or disable recall — falls back to 3."""
+    assert resolve_memory_recall_count({"memory_recall_count": bad}) == 3
+
+
+def test_resolve_clamps_low_so_recall_is_never_disabled():
+    # k<=0 would make `queued >= k`-style logic recall nothing; clamp to the floor.
+    assert resolve_memory_recall_count({"memory_recall_count": 0}) == MEMORY_RECALL_COUNT_MIN
+    assert resolve_memory_recall_count({"memory_recall_count": -5}) == MEMORY_RECALL_COUNT_MIN
+
+
+def test_resolve_clamps_high_so_prompt_cannot_balloon():
+    assert resolve_memory_recall_count({"memory_recall_count": 9999}) == MEMORY_RECALL_COUNT_MAX
+
+
+def test_resolve_truncates_float():
+    assert resolve_memory_recall_count({"memory_recall_count": 2.9}) == 2